-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
type-hints #142
type-hints #142
Conversation
src/ClassNameLayerResolver.php
Outdated
/** | ||
* @param string $className | ||
* | ||
* @return array<string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string[]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -28,5 +28,7 @@ protected function execute( | |||
|
|||
$configurationLoader->dumpConfiguration(); | |||
$output->writeln('depfile <info>dumped.</info>'); | |||
|
|||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can now add the return type hint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -49,5 +49,7 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
} | |||
|
|||
$output->writeln('<info>Deprac is already the latest version.</info>'); | |||
|
|||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/Configuration.php
Outdated
* @param ConfigurationLayer[] $layers | ||
* @param ConfigurationRuleset $ruleset | ||
* @param array $paths | ||
* @param array $excludeFiles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would now drop the docblock. It doesn't add any value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed; replaced array
with string[]
* @param $color | ||
* @param $collectors | ||
* @param ConfigurationCollector[] $collectors | ||
* @param string $name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
{ | ||
# must | ||
yield [ | ||
[ | ||
'must' => [ | ||
['type' => true], | ||
['type' => 'true'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/sensiolabs-de/deptrac/blob/master/src/Collector/CollectorInterface.php#L13-L19 says it should be a string and the value will be used as an array key at https://github.com/sensiolabs-de/deptrac/blob/master/src/CollectorFactory.php#L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, it's ok even if i think it's a bit overkill. getCollectorMock('true', true)
and 'true'
is brainfuck.
i'd prefer casting the bool to a string. there will never be other values than true / false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
truly, mixing strings and booleans (true / false) is not a nice style. we're talking about the type here, and the type has a name. "true" or "false" as a string. Or "boolean", "object", "null" etc. And not if the state is true or false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timglabisch I added some more improvements and fixed the "brainfuck" as you mentioned. Even I could apply the validity with phpstan level 7.
* @param $arguments | ||
*/ | ||
public function __construct($arguments) | ||
public function __construct(array $arguments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to $options
No description provided.