search

aik099 / CodingStandard

The PHP_CodeSniffer coding standard I'm using on all of my projects
BSD 3-Clause "New" or "Revised" License
5 stars 2 forks source link

Mute unused error instead of overriding process() #24

Closed gsherwood closed 10 years ago

gsherwood commented 10 years ago

Instead of overriding the main process() method completley, we can just perform some additional checks and then use the rulset.xml to mute the ShortNoCaptial error that we don't want. Instead, we do our own short comment checks and use different codes so they are not confused.

This sniff isn't 2.x ready yet, but hopefully this will make it easier to become ready.

aik099 commented 10 years ago

Looks good. At least now it's easier to move forward in making this sniff PHPCS 2.x compatible.

I guess this PR is ready for merging and another PR will make code in this one PHPCS 2.x compatible?

gsherwood commented 10 years ago

I guess this PR is ready for merging and another PR will make code in this one PHPCS 2.x compatible?

Yeah, I was going to leave the 2.x modifications for another PR so I can get all the sniffs working in one go.

I have submitted another PR where I made the ArraySniff phpcbf compatible. I can help with that if you care about auto-fixing errors, but I did that PR more as an example to show you the sort of changes required.

aik099 commented 10 years ago

Yeah, I was going to leave the 2.x modifications for another PR so I can get all the sniffs working in one go.

Merging. Thanks, @gsherwood .

I have submitted another PR where I made the ArraySniff phpcbf compatible.

I saw it. Thanks.

I can help with that if you care about auto-fixing errors, but I did that PR more as an example to show you the sort of changes required.

That would be great. I wonder how this fixing functionality can be tested at all? Probably we need 2 files for each sniff:

gsherwood commented 10 years ago

I wonder how this fixing functionality can be tested at all?

The test runner in PHPCS is not able to test external standards as well, so you shouldn't need to bootstrap it any more.

It also checks that all errors/warnings marked fixable are actually fixed in each test file, so the fix testing happens automatically if you use the built-in test runner. I helped the WordPress guys get there one going (they have a lot of tests too) so I can help with your runner as well.

aik099 commented 10 years ago

The test runner in PHPCS is not able to test external standards as well ...

Of that I'm aware. That's why I took part of test suite from PHPCS itself and adapted it to work with any standard (just changed from where standard is included and how to generate unit test class names): https://github.com/aik099/CodingStandard/tree/master/TestSuite .

... so you shouldn't need to bootstrap it any more.

I'm not using it, but rather my copy: https://github.com/aik099/CodingStandard/blob/master/TestSuite/AbstractSniffUnitTest.php

It also checks that all errors/warnings marked fixable are actually fixed in each test file, so the fix testing happens automatically if you use the built-in test runner. I helped the WordPress guys get there one going (they have a lot of tests too) so I can help with your runner as well.

I haven't changed much of the original AbstractSniffUnitTest.php file, just minor refactorings. That would be great. Trick here would be to keep AbstractSniffUnitTest.php backwards compatible.

I've looked at https://github.com/squizlabs/PHP_CodeSniffer/blob/phpcs-fixer/tests/Standards/AbstractSniffUnitTest.php and I see several changes only:

  1. the $phpcsFile->initStandard() method is used instead of $phpcsFile->process()
  2. the fixable error count is compared to made fix count

However what I don't see is check of fixes itself. So if fix should remove 2 spaces for example on line 15, then nobody is asserting that these 2 are removed.

gsherwood commented 10 years ago

No, the unit testing system doesn't directly check that, for example, 2 spaces were removed from line 5. It also doesn't check that a specific error message was generated on line 5; just that an error message was generated.

In practice, this hasn't caused any issues for me, so I've never needed to make every check more specific. Given that only one sniff is running on a single file, if all the error were found and all the errors are now fixed, there is a very high chance that everything is working.

Of course, you can go ahead and write a strict diff-based unit tester for yourself. If you go that way and need anything, just let me know.

I'll see if I can get this sniff working in the 2.x version this week hopefully.

aik099 commented 10 years ago

It also doesn't check that a specific error message was generated on line 5; just that an error message was generated.

Of that I'm aware. It just counts error/warning messages per line to see if it's all works and desired errors were discovered.

Given that only one sniff is running on a single file, if all the error were found and all the errors are now fixed, there is a very high chance that everything is working.

For finding errors how the error is found doesn't really matter since it's found eventually. However when we're fixing things it's good practice to check on a result of fixing to see if we really fixed what we wanted, e.g.

Of course, you can go ahead and write a strict diff-based unit tester for yourself. If you go that way and need anything, just let me know.

Yes, some guidance like:

I'll see if I can get this sniff working in the 2.x version this week hopefully.

Thanks, send PR when you'll have it.

gsherwood commented 10 years ago

initStandard() just does the basics to get PHPCS ready to check files so that the tests can then specify a file at a time. The tests don't need PHPCS to actually do a full process run; they just need a single file checked without some of the command line restrictions being enforced.

To generate a diff, you'd use the Fixer object. Something like:

$phpcsFile->fixer->fixFile();
$fixable = $phpcsFile->getFixableCount();
if ($fixable > 0) {
    echo 'Fixing failed';
} else {
    $diff = $phpcsFile->fixer->generateDiff;
}

Also, I've just submitted PR #29 for 2.x support. I've got a backlog of work now so I haven't done a heap of testing on it, but I think it is working.

aik099 commented 10 years ago

I've made suggested changes in #32 and it looks good so far. Combined with #30 we can now really see effects of these changes on Travis.

gsherwood commented 9 years ago

However what I don't see is check of fixes itself. So if fix should remove 2 spaces for example on line 15, then nobody is asserting that these 2 are removed.

Just an update to let you know that I rewrote a quite important sniff (the one that checks indentation) and decided to implement your idea of diffing the fixed file against a known good copy. The commit is here: https://github.com/squizlabs/PHP_CodeSniffer/commit/23920d18e4293e6e773e46b59d01675cccb26c04

aik099 commented 9 years ago

In https://github.com/aik099/CodingStandard/blob/master/TestSuite/AbstractSniffUnitTest.php#L96 I've stored the expected generated diff in "*.diff" file and compared that to the actual generated diff.

Not sure which solution is better though.

gsherwood commented 9 years ago

It's basically the same thing, except that I've used a .fixed extension and I'm using the PHP_CodeSniffer_Fixer class to generate a diff that can be printed to screen to see where the fixes have not been applied correctly.

I just wanted to check in and let you know about the change considering I previously said I wasn't planning on adding it.

aik099 commented 9 years ago

The different is what actions developer need to take when mismatch is detected:

I think I'll merge your changes in my AbstractSniffUnitTest.php then.

Good to know that you did that, because that eases problem detection process during sniff evolution.