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

Added auto fixes for the ArraySniff #25

Closed gsherwood closed 10 years ago

gsherwood commented 10 years ago

The change will work on both the 1.x and 2.x versions of PHP_CodeSniffer because it checks for the existence of $phpcsFile->fixer, which is a good way to tell what version you are in.

aik099 commented 10 years ago

Thanks, @gsherwood .

aik099 commented 10 years ago

Ah, the Scrutinizer CI complains a lot about usage of PHPCS 2.x methods in PHPCS 1.x sniff: https://scrutinizer-ci.com/g/aik099/CodingStandard/inspections/274250f5-3e04-4889-9212-05e88ae05b97/issues/files/CodingStandard/Sniffs/Array/ArraySniff.php?status=new&orderField=path&order=asc

As for Travis I can probably run tests on both PHPCS 1.x and PHPCS 2.x versions to get combined coverage (created #27 as a reminder).

gsherwood commented 10 years ago

Seems like it should ignore them given there is a check to ensure the object exists before using it. There isn't anything actually wrong with that code, so it would be good if it could ignore it.

I could use a method_exists check, but that might be slower. I'm also not sure if Scrutinizer would then stop reporting errors, but you could try that instead.

On Friday, 15 August 2014, Alexander Obuhovich notifications@github.com wrote:

Ah, the Scrutinizer CI complains a lot about usage of PHPCS 2.x methods in PHPCS 1.x sniff: https://scrutinizer-ci.com/g/aik099/CodingStandard/inspections/274250f5-3e04-4889-9212-05e88ae05b97/issues/files/CodingStandard/Sniffs/Array/ArraySniff.php?status=new&orderField=path&order=asc

As for Travis I can probably run tests on both PHPCS 1.x and PHPCS 2.x versions to get combined coverage.

— Reply to this email directly or view it on GitHub https://github.com/aik099/CodingStandard/pull/25#issuecomment-52282918.

Greg Sherwood Chief of Engineering gsherwood@squiz.com.au

Squiz Labs Pty. Ltd. Level 2, 60 Macquarie Street Parramatta 2150 P +61 2 9045 2800 W www.squizlabs.com

aik099 commented 10 years ago

Seems like it should ignore them given there is a check to ensure the object exists before using it.

  1. we check for fixer property presence, but then call addFixableError method based on assumption that it should also exist when fixer property does
  2. PhpAnalyzer (on Scrutinizer CI) knows that we get PHPCS 1.x via composer.json and uses PHP_CodeSniffer_File class declaration from there to determine that it doesn't have fixer property either
  3. I think that isset($phpcsFile->fixer) === false check isn't valid way for property presence check because of there is NULL in that fixer property then check will fail as well. Maybe you wanted to check exactly that (fixer property exists and something is in it).

I could use a method_exists check, but that might be slower. I'm also not sure if Scrutinizer would then stop reporting errors, but you could try that instead.

I'd better hide these errors on Scrutinizer CI side, since you don't see sniffs that want to work on both PHPCS 1.x and PHPCS 2.x at same time :smile:

gsherwood commented 10 years ago

we check for fixer property presence, but then call addFixableError method based on assumption that it should also exist when fixer property does

Yeah I know, but you can make that assumption in your code given that PHPCS does it in a hundred places as well :)

I understand why PhpAnalyzer says the method may not exist. In this case, it's just backward compatibility code that it can't understand, so would be great if you could get Scrutinizer to hide the errors.

aik099 commented 10 years ago

I understand why PhpAnalyzer says the method may not exist. In this case, it's just backward compatibility code that it can't understand, so would be great if you could get Scrutinizer to hide the errors.

Done as asked.