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
more code grooming #7233
more code grooming #7233
Conversation
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.
What I removed felt important but I couldn't get how it could be useful
We can leave it for another day then. I imagine it would be quite a pain to put it back half a year later should it actually break something.
@@ -55,8 +55,6 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev | |||
) { | |||
$filter_type_type = $second_arg_type->getSingleIntLiteral(); | |||
|
|||
$filter_type = null; |
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 actually either leave it here or add a default case to the switch below.
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.
Did you see this line a little above? https://github.com/vimeo/psalm/pull/7233/files/0b3e1299decdd75fe6cf38acd88391cfea323516#diff-76c8a38ce19c782b153d6a5cbededfe163697de0e37f2f40559f3f07f8eaf318L50
It was just initialized twice
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.
Ok then.
$lines[$k] = str_replace("\t", ' ', $line); | ||
$lines[$k] = preg_replace('/^ *\*/', '', $line); |
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.
It looks like a bug actually, the tab expansion was futile.
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.
Yeah, this was definitely a bug. Forgot to mention it in the PR. I fixed it the way I think it was intended, but I'm not sure what it was designed to do in what case though...
It's pretty minor refactoring, except for what I removed in ParseTreeCreator. What I removed felt important but I couldn't get how it could be useful and tests seems to prove me right...