test(phpcs): Check for unused variables in PHPCS itself

klausi commented 5 months ago

I found an used variable in src/Standards/Squiz/Sniffs/Functions/MultiLineFunctionDeclarationSniff.php . We should check for unused variables with automated testing.

Description

We can check for unused variables with https://github.com/slevomat/coding-standard/blob/master/doc/variables.md#slevomatcodingstandardvariablesunusedvariable .

Not sure if you want to have a dependency on Slevomat for development? This pull request demonstrates a possibility for the approach, but I did not finish fixing all unused variables yet.

Let me know if this direction makes sense and then I can fix them!

Suggested changelog entry

not needed, as this is an internal fix and will not be visible for users.

Related issues/external references

Types of changes

Bug fix

PR checklist

[x] I have checked there is no other PR open for the same change.
[x] I have read the Contribution Guidelines.
[x] I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
[x] I have added tests to cover my changes.
[x] I have verified that the code complies with the projects coding standards.
[x] [Required for new sniffs] I have added XML documentation for the sniff.
[x] [Required for new files] I have added any new files to the package.xml file.

jrfnl commented 5 months ago

@klausi Thanks for this PR, I can accept the code changes, but not the change to CI/dependency addition.

klausi commented 5 months ago

OK, I assume you also don't want to fork the Slevomat sniff into PHPCS to avoid the dependency?

Anyway, I could setup an external CI to check the PHPCS master branch once per day with cron and then send you manual pull requests whenever new unused variables are introduced by accident. It should not happen often anyway.

I will fix the reported instances first to see if there are any false positives.

jrfnl commented 5 months ago

OK, I assume you also don't want to fork the Slevomat sniff into PHPCS to avoid the dependency?

Correct, see the CONTRIBUTING guide on copyright.

Aynway, I could setup an external CI to check the PHPCS master branch once per day with cron and then send you manual pull requests whenever new unused variables are introduced by accident. It should not happen often anyway.

I appreciate the offer, but no need, I can add it to my local phpcs.xml overload file which contains various other extra sniffs as well and based on which I once in a while send in a clean up PR, like #339. I think that should be enough.

I will fix the reported instances first to see if there are any false positives.

Appreciated, I'll have a look. One word of warning: please be careful, not all code is covered by tests.

klausi commented 4 months ago

Here is the Slevomat config I used to verify that all unused variables are fixed: https://github.com/klausi/PHP_CodeSniffer/commit/3386955e3b2c3d2052f8b73a4c305adec1a375ae

jrfnl commented 4 months ago

@klausi Had a chance to gather your thought on my remarks yet ?

klausi commented 4 months ago

Sorry, got busy with other things, but your feedback looks good! Will work it in when I have time again.

jrfnl commented 4 months ago

@klausi Take your time. I just wanted to check in.

klausi commented 3 months ago

I'm done with the reverts, let me know if you need anything else!

PHPCSStandards / PHP_CodeSniffer