Closed gsherwood closed 10 years ago
I've updated AbstractSniffUnitTest.php
to work with both PHPCS 1.x and PHP 2.x in #32. Please rebase your PR on top of master
after I've merged it so we can see results on Travis.
I've rebased quickly, but I'm getting too busy to take a good look into it. I think you'll need to keep going with this. Hopefully the changes I've got in here will help.
Feel free to contact me if you have any questions or need any help though. Sorry if I'm a bit slow to respond.
It seems that you haven't rebased on master
but on the other PR branch instead (where I've made AbstractSniffUnitTest.php
changes). I know this because only your commits should be present in commit list, but now mines are present as well.
If it's ok with you I'll take your commit and manually apply to my own branch. Thanks to Git your name will still be present on a commit (as author) and I will be set as commiter.
In fact rebasing is pretty easy:
master
branch of your fork is up to date by pulling git pull upstream
(presuming that upstream
is the name of my repo and origin
is name of your fork's clone)git rebase master
to rebasegit push -f origin
to update your PR branch on GitHubI've travis build marked as passed even if PHPCS 2.x tests are failing. And they still fail: https://travis-ci.org/aik099/CodingStandard/jobs/33557873
Errors might suggest that Squiz function comment sniff 2.0 isn't backwards compatible with it's 1.5.4 counterpart :(
Yeah, go ahead and apply the changes any way you want. I did a nice rebase but then changed things again and had to rebase to merge some commits, hence the current state. But both were rebased against master. The second one just forced me to pick those commits when I wanted to squash the new one.
And yes, the errors may not be exact between 1.x and 2.x due to the other sniffs that have changed and how the base class has also changed (some errors removed, some new ones added). You'll need to see if you are happy with the errors you are getting now. If not, you'll just need to keep changing the 2.x version of the sniff until you are.
Yeah, go ahead and apply the changes any way you want.
Will do, thanks.
And yes, the errors may not be exact between 1.x and 2.x due to the other sniffs that have changed
Was this an intentional decision or just a side effect of massive commenting sniff refactoring?
Was this an intentional decision or just a side effect of massive commenting sniff refactoring?
I intentionally removed all the checks that forced specific tag orders and added checks to ensure the same comment tags were grouped together. There may have been other changes in there as well, based on changes to the Squiz coding standard that I use.
So now I can have @return
before @param
I suppose and nobody will complain?
Can you please link to one of the checks that were removed so I can easily pinpoint other ones as well?
Some of the errors have been removed, while others moved here: https://github.com/squizlabs/PHP_CodeSniffer/blob/phpcs-fixer/CodeSniffer/Standards/Generic/Sniffs/Commenting/DocCommentSniff.php
The same is true for Class and File comments. They've all been modified in the same sort of way. The tag ordering bit is the biggest change.
After in depth comparison of error list produced by PHPCS 1.x Squiz FunctionComment's sniff and 2.x Squiz FunctionComment's sniff + DocCommentSniff I've found several things:
@param
validations are done individually and not in relation to next parameter after validated oneThis caused some of errors to shift and adaptations needed to make sniff tests to pass.
Also some new validations were added. And I found only 1 error about @return
and @param
grouping that was actually reported on @param
and not on @return
. After all I managed to get test to pass.
Hard part was to identify which old errors correspond to which new ones considering not only error text change, but also sniff error code change as well.
Thanks to the fact, that we combined 2 sniffs in one the produced error list wasn't that much different, hence the error diff pretty small: https://github.com/aik099/CodingStandard/commit/de70c21d52a82386b798dc2b2110767f6326b265
Closing in favor of #35 (contains commits from this PR as well).
Maybe we should have taken a bit different approach regardless FunctionComment
sniff. Right now we disregard DocComment
sniff for general use and only use it for function comment. Maybe instead we shouldn't have:
Maybe we should have taken a bit different approach
Sure, that would work just as well.
These wont work correctly in 2.x until after the Generic sniffs are included instead of excluded in the ruleset.xml file as the new DocComment sniff will be executed twice until then. But they pass the tests on the 1.x version and hopefully give a good idea of how to maintain backwards compatibility.