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

ci: Fix strict types sniff #857

Merged
merged 7 commits into from
Nov 19, 2024

Conversation

tomasnorre
Copy link
Contributor

Resolves the problem from Updating to "squizlabs/php_codesniffer": "3.11.0"

@tomasnorre
Copy link
Contributor Author

I'm a little confused that the exclude isn't respected anymore. From a quick glance all errors are from example.php or *Test.php which both should be excluded.

<rule ref="src/Sniffs/StrictTypes/ExplainStrictTypesSniff.php">
  <exclude-pattern>*/*Test\.php</exclude-pattern>
  <exclude-pattern>*/.meta/*\.php</exclude-pattern>
  <exclude-pattern>src/*</exclude-pattern>
  <exclude-pattern>contribution/*.php</exclude-pattern>
</rule>

@mk-mxp
Copy link
Contributor

mk-mxp commented Nov 14, 2024

Well, looks like the sniff name generation has gone wrong and so the excludes are not applied for our sniff. Would you mind opening an issue at PHPCodeSniffer and report the problem?

jrfnl and others added 4 commits November 15, 2024 10:44
Note: I've not changed the `composer.json` file, but it could also be considered to leave the sniff namespace the same and to change the `"Exercism\\": "src",` config in the `composer.json` file to `"Exercism\\": "src\\Exercism",`.
The class name is already translated to a sniff name, so no need to duplicate it. Also not a good idea to use backslashes in the error code as that makes configuration in the XML file more fiddly.
…properly

Properly comply with PHPCS naming conventions
@tomasnorre
Copy link
Contributor Author

It's now working ready for review.

I'm personally self in doubt about the src/Exercism/ directory, if that should be called something else.

@tomasnorre tomasnorre requested a review from mk-mxp November 15, 2024 12:22
@tomasnorre tomasnorre added x:action/fix Fix an issue x:knowledge/elementary Little Exercism knowledge required x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows) x:size/small Small amount of work x:rep/small Small amount of reputation labels Nov 15, 2024
@mk-mxp mk-mxp added x:knowledge/none No existing Exercism knowledge required and removed x:knowledge/elementary Little Exercism knowledge required labels Nov 16, 2024
Copy link
Contributor

@mk-mxp mk-mxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which version of PHP CodeSniffer is required as a minimum, so that the name change works with the exclude-pattern? This should be set in composer.json "require-dev": { "squizlabs/php_codesniffer": "^3.11.?" } accordingly.

phpcs.xml Show resolved Hide resolved
@tomasnorre
Copy link
Contributor Author

Which version of PHP CodeSniffer is required as a minimum, so that the name change works with the exclude-pattern? This should be set in composer.json "require-dev": { "squizlabs/php_codesniffer": "^3.11.?" } accordingly.

3.11.1, I'll update composer.json too.

@mk-mxp mk-mxp added x:size/medium Medium amount of work x:rep/medium Medium amount of reputation and removed x:size/small Small amount of work x:rep/small Small amount of reputation labels Nov 18, 2024
@tomasnorre tomasnorre requested a review from mk-mxp November 18, 2024 19:25
@mk-mxp mk-mxp merged commit 4782c2b into exercism:main Nov 19, 2024
12 checks passed
@tomasnorre tomasnorre deleted the ci-fix-strict-types-sniff branch November 19, 2024 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/fix Fix an issue x:knowledge/none No existing Exercism knowledge required x:rep/medium Medium amount of reputation x:size/medium Medium amount of work x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants