-
Notifications
You must be signed in to change notification settings - Fork 11
Add SpecifyArgSeparatorFixer to make sure http_build_query is properly used #18
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
Conversation
@TomasVotruba
TomasVotruba
left a comment
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.
Very nice work on first Fixer. I wish mine was to simple and easy to understand :D
Since you don't use pure PHP CS Fixer, I have one more tip: I do use ECS Tester package, to simulate 1:1 ECS behavior (less WTF, as it's the same as final code)
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 not convetion "Lmc\\CodingStandard\\Tests\\": "tests/"?
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.
We use the same namespace for Class and its corresponding test.
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 know :D but why not convention?
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.
AFAIK both of the approaches are widely used, so I don't consider any of them as "general convention".
And why it should be in standalone namespace in the first place? I don't see a reason for this.
The same namespace means you don't need to import the class under test namespace, also it makes sense to me to have class and its tests in the same "module" (namespace), because they belongs to each another.
But its still more about personal preference...
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.
Similar unconvention as putting src and tests in the same directory, causes in php cs fixer only troubles. 80 % of php-scoper setup took me only this to solve.
It's really painful to work with such packages, where maintainer don't see behing the package common use.
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.
Sorry, I don't understand :). This is the way we use it for years and never heard of single issue. And it is not any "hack", just usual setting of Composer autoloading. But yeah, I can imagine this could cause trouble when developing some low-level stuff.
BTW src/ and tests/ in the same (root) directory are used almost everywhere except packages which are then splited to sub-packages (like Symfony). I find most inconvenient when source is not in standalone directory and mixed with Tests directory on the same level. Setting code-coverage or phpcs etc. blacklist/whitelist is nightmare in this case.
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.
Again, open source code is very different to closed one.
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.
True, but I don't think the difference is in the way autoloading is set up :). But this seems like a topic to pub/meetup, so let's don't forget it next time we meet :-D . I'd like to hear about the stuff you must dealt with :).
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'd expect this line rather (even) at isRisky() method
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'd add all this to "shouldSkip()" method, it's not relevant to fixer process at all
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 did you choose this new approach?
I'd use one of standards: php-cs-fixer or ECS. How could you copy this class to easy-coding-standard.yml from CLI output to skip it?
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.
"The name must be all lowercase and without any spaces." - this is in phpDoc.
But I saw you use class name. So I combined them both :-D. What is the best solution? Where is this name being used?
ECS is depending just on the class name FQCN, or it somehow works with this?
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 is this here? Should be above the fix funciton
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.
getNextTokenOfKind() can by annotation return null as well. So to adhere type safety, this case must be somehow handled.
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.
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'd be happy to check after that, there is lot of outsourcable code now
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.
:-D I was today looking for something like this like an half an hour and did not find it. So this makes part of the code unnecessary, I will refactor it with use of the ArgumentsAnalyzer. Thanks!
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.
This section very difficult to read. What does it do?
I'd expect it to be bellow, with other "modifying" code
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.
Real code might be also helfpul while reading this, e.g.:
// Add third parameter (arg separator): ", &")I always add it because it's much more readable than custom Tokens
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.
asssertStringEqualsFile() or how its called
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.
While using dataProvider, it makes sense to use one file per case.
Imagine you have bug in Wrong.txt on line 25 - which one of these cases it?
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.
@TomasVotruba True, but there is overhead and is also harder to see what is covered and what not. Maybe rather write inline PHP for each case (as some fixer tests do).
You will see which line of code is broken by phpunit's diff, right?
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, how do you run broken code only?
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 dont (I guess).. but why would you run just part of tests?
If you are developing something new, you can run single test case or I think even one perticular provider value test case in PHPStorm..
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 do that too, but how to run specific one line in this single file?
8f1eca0 to
bfbdc36
Compare
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 not to encapsulate those commands into all script?
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 need phpunit with specific settings for CI (to generate clover). But I may do something like "all-ci" or so.
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 consider using lmc/coding-standards here as well?
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 :)? This is the package itself 🙃
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.
Are you sure with return? IMHO this should be just continue, or am I wrong?
Imagine code
function foo($args) { $this->addCallback('http_build_query'); // this is not a function call return http_build_query($args); // this is a function call, but isn't it skipped due previous `return`? }
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.
Nice catch, fixed. 👍
Tests actually hide this, because the case with definition of function http_build_query were the last, so the bug did not show :). So I rearranged them.
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, would this Token find usage of http_build_query(..., "&");? You explicitely awaits it is in single quotes.
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.
Good point. Priority is now set to -1, ie. it will run after SingleQuoteFixer.
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.
Just saying.. but this looks very confusing to me (+ 1, ++$index,...).
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.
mm.. I guess following code is a nightmare for Fixer author, but have you considered something like this?
http_build_query([], null, ('&'));
Changes done & refactored to use ArgumentsAnalyzer. cc @TomasVotruba @MortalFlesh et al. 🙏
In which commit did you add the ArgumentAnalyzer? The commit messages are not very useful :(
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'm not fan of negative bool methods or variables, it is confusing to use/read .. I'd prefer function isFunctionCall()
and then if (!$this->isFunctionCall())..
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.
Neither am I, but this function in fact doesn't prove it is function call when you just negate the logic - there are IMHO many edge cases. What it can is just to detect the main scenarios when it is not a function call :). So from my POV, the other name would be inaccurate and misleading :).
🤷
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.. 🤔
Ok in this case I think it might be ok..
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.
@MortalFlesh I thought about it again and changed it in the end... The objection still applies, but maybe its less harm like this 🤷♂️ .
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 there is no type?
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.
How can this pass the CI?
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.
😱
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.
@TomasVotruba It seems phpdoc_add_missing_param_annotation (aka PhpdocAddMissingParamAnnotationFixer) does not work as I expected :-(. It only works if there is already at least something in phpdoc.
Do you know some check which takes care of this?
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 I would name it rather $argument, instead of $argumentIndex, which I'd expect holds just an index of the argument.
And maybe name it explicitely $thirdArgument..
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.
So is this int (index/position) or some array/object ($argument) ?
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.
The content is [startIndex => endIndex]. IMO argument is still misleading - it contains just the indexes, not the argument itself 🤔
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 name these BlockInfo
In which commit did you add the ArgumentAnalyzer? The commit messages are not very useful :(
You can easily go to Files changed > Changes from: > Select "Since last review" or something like this (it is selected by default) to see what has changed.
We don't write new commit messages when you only adjust things in response to code-review feedback, because commit messages should be about behavior change in the code, to make the history help with long term maintainability of the code (it helps a lot when using git blame etc.). There already is commit about adding the fixer, so unless any new commit in this PR will add/change some other behavior, the changes are just associated to the original commit via fixup and will be rebased and squashed before merge.
Interesting article about this topic: https://arialdomartini.wordpress.com/2012/09/03/pre-emptive-commit-comments/ (especially "Tell me what the software does (not what you did)" part) and also of course https://filip-prochazka.com/blog/git-fixup about fixup commits.
TomasVotruba
commented
May 11, 2018
It's just uncommon and confusing in open source, that's all.
...ined in http_build_query() (fixes #17)
77b31af to
c834dd8
Compare
Merged, thanks @TomasVotruba @MortalFlesh a lot for your great feedback! 🥇
Fixes #17 .
To be honest, this is the first Fixer I've ever written. So any suggestions especially regarding usage of php-cs-fixer internals (the tokens stuff etc.) are welcome. Also I may overlook some simpler way how to accomplish this, so I'm open for suggestions.
Cc @TomasVotruba - I would be grateful to hear some feedback on the Fixer from you as well ❤️ :-)