test(phpcs): Check for unused variables in PHPCS itself #446
Conversation
klausi
changed the title
|
@klausi Thanks for this PR, I can accept the code changes, but not the change to CI/dependency addition. |
|
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. |
klausi
changed the title
Correct, see the CONTRIBUTING guide on copyright.
I appreciate the offer, but no need, I can add it to my local
Appreciated, I'll have a look. One word of warning: please be careful, not all code is covered by tests. |
|
Here is the Slevomat config I used to verify that all unused variables are fixed: klausi@3386955 |
There was a problem hiding this comment.
Thanks for updating the PR @klausi . I've left various comments inline to have a look at. For now, I tend to err on the side of caution with some things as - especially on the framework side - there are significant gaps in tests/documentation, which may make certain changes more risky and others undesirable as they may lead to more detective work being needed at a later point in time.
Let me know what you think.
src/Standards/Generic/Sniffs/ControlStructures/DisallowYodaConditionsSniff.php
Outdated
Show resolved
Hide resolved
|
@klausi Had a chance to gather your thought on my remarks yet ? |
|
Sorry, got busy with other things, but your feedback looks good! Will work it in when I have time again. |
|
@klausi Take your time. I just wanted to check in. |
|
I'm done with the reverts, let me know if you need anything else! |
* test(phcs): Check for unused variables in PHPCS it self * Manually fixed all unused varaibles * Revert Slevomat dependency * Revert array_keys changes * revert changes that should stay
Add initial set of tests for the `Common::getSniffCode()` method. Related to 146 Related to [review comment in PR 446](#446 (comment)).
* Remove the use of `array_pop()` in favour of directly referencing the required "parts" by their index in the array. * Remove the unused `$sniffDir` variable. * Remove the unnecessary `$code` variable. Related to [review comment in PR 446](#446 (comment)).
* Remove the use of `array_pop()` in favour of directly referencing the required "parts" by their index in the array. * Remove the unused `$sniffDir` variable. * Remove the unnecessary `$code` variable. Related to [review comment in PR 446](#446 (comment)).
* Remove the use of `array_pop()` in favour of directly referencing the required "parts" by their index in the array. * Remove the unused `$sniffDir` variable. * Remove the unnecessary `$code` variable. Related to [review comment in PR 446](#446 (comment)).
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
package.xmlfile.