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

refactor(FullyQualifiedNamespace): Replace sniff in favor of a sniff from Slevomat #199

Draft
wants to merge 3 commits into
base: 8.3.x
Choose a base branch
from

Conversation

klausi
Copy link
Collaborator

@klausi klausi commented Apr 30, 2023

Issue: https://www.drupal.org/project/coder/issues/3214374

The goal would be to delete our FullyQualifiedNamespace sniff and switch to using the one from Slevomat.

Was not able to complete this yet because of a bug in Slevomat slevomat/coding-standard#1564

@jonathan1055
Copy link
Contributor

jonathan1055 commented May 1, 2023

You've probably realised this, but the failing tests for standards is because the new file tests/Drupal/good/GoodNamespace.php is not excluded when checking Coder's own files:

    <!-- Test files that should not be checked. -->
    <exclude-pattern>tests/*\.(inc|css|js|api\.php|tpl\.php)$</exclude-pattern>
    <exclude-pattern>tests/*/(good|bad)\.php$</exclude-pattern>
    <exclude-pattern>tests/*/drupal(6|7|8)/*\.php$</exclude-pattern>

The patterns above seem to exclude nearly all files in the tests/good/ folder but not everything. Am I right in thinking that all files in the tests/good folder should follow Drupal standards, and hence they are excluded because Coder uses a different customised set for it's own files? If so, we could exclude everything from tests/good in one statement, rather than try to cater for most files over the three patterns.

Sorry if this is wrong, I've not looked in great detail at what is not excluded from the above three.

@klausi
Copy link
Collaborator Author

klausi commented May 2, 2023

Yep, this is not ready for review yet, was just uploading my work in progress here.

@jonathan1055
Copy link
Contributor

Sorry, yes of course. But I've done some learning in the meantime, I wanted to see if we could exclude everything in /good except the GoodUnitTest.php but I could not get a neagative pattern to work, so the easiest thing would be to add GoodNamespace into the tests/*/(good|GoodNamespace|bad)\.php$ pattern

@klausi
Copy link
Collaborator Author

klausi commented May 6, 2023

Created upstream bug report at slevomat/coding-standard#1570

@klausi
Copy link
Collaborator Author

klausi commented May 6, 2023

That bug was resolved, yay!

Now some test fails remain where Slevomat gets confused by multiple use statements, it somehow thinks one is a constant.

Not sure if we should simply drop this test case from Coder and not care about it anymore, or if we should try to improve the Slevomat sniff.

@klausi
Copy link
Collaborator Author

klausi commented Jun 9, 2023

Reported the constant confusion upstream: slevomat/coding-standard#1581

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.

2 participants