Skip to content
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

fix for #3 & #3 (does not touch SniffTest.php) #5

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bkdotcom
Copy link

@bkdotcom bkdotcom commented Oct 26, 2021

refactor MaximumComplexity Sniff & Analyzer.php to utilize "$phpcsfile" and how nesting level increment is calculated

fixes #2 and #3 but SniffTest fails

…e" and how nesting level increment is calculated
@bkdotcom bkdotcom changed the title refactor MaximumComplexity Sniff & Analyzer.php to utilize "$phpcsfil… fix for #3 & #3 (does not touch SniffTest.php) Oct 26, 2021
@bkdotcom
Copy link
Author

minor note... the scrutinizer coding standard that is failing violates PSR-12
See "Closures"
https://www.php-fig.org/psr/psr-12/

@Rarst
Copy link
Owner

Rarst commented Nov 1, 2021

  1. Still unrelated code style changes, only what matters for the fix, pretty please.
  2. I don't follow the need to switch to file object, it doesn't seem to be used except for immediately getting the same token stream from?
  3. I tried to extract just the logic changes, the switch is fixed with them, but it broke nesting for lambda (test numbered 4).

@bkdotcom
Copy link
Author

bkdotcom commented Nov 1, 2021

@Rarst
I have removed non-important changes.
All AnalyzerTest sniffs are passing (including the Closure test - function4.php.inc)

PHP_CodeSniffer\Files\File object is also utilized in
isIncrementingToken

$nextToken = $this->phpcsFile->findNext(Tokens::$emptyTokens, $position + 1, null, true);
if ($nextToken === false || $tokens[$nextToken]['code'] !== T_SEMICOLON) {

instead of simply looking at $position + 1
This addresses issue #2

@Rarst
Copy link
Owner

Rarst commented Nov 1, 2021

PHP_CodeSniffer\Files\File object is also utilized in is IncrementingToken

Ah, must have missed that.

Checked branch as is now.

  1. Why does it change phpunit.xml ? That causes warning too:
PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

  Warning - The configuration file did not pass validation!
  The following problems have been detected:

  Line 3:
  - Element 'coverage': This element is not expected.

  Test results may not be as expected.
  1. Multiple tests error out on a constant:
Error : Undefined constant "PHP_CodeSniffer\PHP_CODESNIFFER_CBF"
 C:\server\coding-standards\phpcs-cognitive-complexity\vendor\squizlabs\php_codesniffer\src\Config.php:961
 C:\server\coding-standards\phpcs-cognitive-complexity\vendor\squizlabs\php_codesniffer\src\Config.php:434
 C:\server\coding-standards\phpcs-cognitive-complexity\vendor\squizlabs\php_codesniffer\src\Config.php:335
 C:\server\coding-standards\phpcs-cognitive-complexity\tests\AnalyzerTest.php:62
 C:\server\coding-standards\phpcs-cognitive-complexity\tests\AnalyzerTest.php:29
  1. One of the tests errors on some array:
Undefined array key 0
 C:\server\coding-standards\phpcs-cognitive-complexity\vendor\squizlabs\php_codesniffer\src\Files\File.php:1233
 C:\server\coding-standards\phpcs-cognitive-complexity\src\CognitiveComplexity\Sniffs\Complexity\MaximumComplexitySniff.php:46
 C:\server\coding-standards\phpcs-cognitive-complexity\tests\SniffTest.php:61

@bkdotcom
Copy link
Author

bkdotcom commented Nov 1, 2021

@Rarst
phpunit.xml has been reverted

it was updated due to this message

Warning: Your XML configuration validates against a deprecated schema.
Suggestion: Migrate your XML configuration using "--migrate-configuration"!

Here's my phpunit 8 output

$ phpunit8 tests/AnalyzerTest.php
PHPUnit 8.5.9 by Sebastian Bergmann and contributors.

.......... 10 / 10 (100%)

Time: 274 ms, Memory: 16.00 MB

OK (10 tests, 10 assertions)

@Rarst
Copy link
Owner

Rarst commented Nov 1, 2021

Gotcha, are you trying to run with PHPUnit 9? Scrutinizer is still set up against 8, so I am running with that locally as well. Can be updated at some point, but not related to the issue.

@Rarst
Copy link
Owner

Rarst commented Nov 1, 2021

Here's my phpunit 8 output

Hm, which PHP CS version? Are you installing according to lock file or updating to latest?

@bkdotcom
Copy link
Author

bkdotcom commented Nov 1, 2021

Yes.. I guess it's just 9 that throws that warning....

I just tested with PHPUnit 8.5.9 (see updated previous comment) and it's working

@bkdotcom
Copy link
Author

bkdotcom commented Nov 1, 2021

re phpcs version

$ vendor/bin/phpcs --version
PHP_CodeSniffer version 3.5.3 (stable) by Squiz (http://www.squiz.net)

which is what's in composer.lock

¯\_(ツ)_/¯

@Rarst
Copy link
Owner

Rarst commented Nov 1, 2021

Scrutinizer is currently failing on the constant thing too (tad different message, it's higher severity in PHP 8 locally).

Not sure what's up with it, but PHP CS seems to expect it after the changes made. Probably could be just defined to false since we aren't doing anything CBF related?

@bkdotcom
Copy link
Author

bkdotcom commented Nov 1, 2021

Updated tests/bootstrap.php

// The below two defines are needed for PHPCS 3.x.
if (defined('PHP_CODESNIFFER_CBF') === false) {
    define('PHP_CODESNIFFER_CBF', false);
}

found snippet when googling "undefined PHP_CODESNIFFER_CBF"

@Rarst
Copy link
Owner

Rarst commented Nov 2, 2021

Ok, that exchanged for a new error in Scrutinizer:

PHP_CodeSniffer\Exceptions\DeepExitException: ERROR: option "--coverage-clover=clover.xml" not known

Given that is a parameter being used for PHPUnit, PHP CS shouldn't even be looking at that.

Think the changes to file parsing might be confusing PHP CS about running from a regular command line call, while that's not what is happening? Why were they necessary? (Ok, I see that was done to swap from tokens as result to the File object).

@bkdotcom
Copy link
Author

bkdotcom commented Nov 4, 2021

I'm not sure what's going on with the coverage tests....
But I did encounter a computation bug... fix and new test scenario committed

@Rarst
Copy link
Owner

Rarst commented Nov 4, 2021

It looks to me like PHP CS code yanks arguments from command line and gets confused what those are (since command line is for PHPUnit actually). I am not familiar with its boot process to say why that happens or how could that be adjusted to not happen.

@Rarst Rarst self-assigned this Jun 6, 2022
@Rarst
Copy link
Owner

Rarst commented Jul 12, 2022

Ugh, busy year... War and stuff. :)

So, I ditched Scrutinizer and moved tests setup to GitHub actions and more careful PHP 7.2 + PHPUnit 8 set up (for minimal supported requirements).

For whatever reason now I have tests fail locally as well, the changes to file loading do seem to cause PHP CS to initialize badly and fall apart, one way or another.

If you are still interested, could you please refresh this on top of current master (rebase the branch or merge master into it, should work too, I think)? That should get us tests running and reporting inline in PR here.

@Rarst Rarst removed their assignment Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve sniff implementation to be code style agnostic
2 participants