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

klausi · 2024-04-13T10:48:54Z

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

I have checked there is no other PR open for the same change.
I have read the Contribution Guidelines.
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).
I have added tests to cover my changes.
I have verified that the code complies with the projects coding standards.
[Required for new sniffs] I have added XML documentation for the sniff.
[Required for new files] I have added any new files to the package.xml file.

jrfnl · 2024-04-13T15:25:14Z

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

klausi · 2024-04-14T08:38:05Z

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.

src/Filters/Filter.php

src/Files/File.php

jrfnl · 2024-04-14T16:54:42Z

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.

jrfnl

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/Reports/Full.php

src/Standards/Generic/Sniffs/ControlStructures/DisallowYodaConditionsSniff.php

src/Tokenizers/CSS.php

src/Util/Common.php

src/Runner.php

tests/Standards/AbstractSniffUnitTest.php

tests/bootstrap.php

src/Standards/Squiz/Sniffs/PHP/CommentedOutCodeSniff.php

jrfnl · 2024-05-16T10:32:05Z

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

klausi · 2024-05-16T11:40:15Z

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

jrfnl · 2024-05-16T11:51:58Z

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

klausi · 2024-05-31T10:42:03Z

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

jrfnl

@klausi Thank you for working on this. All looking good now.

I've opened up issue #521 as a follow up and will open a couple of PRs with follow ups as well (related to some of the reverts).

* 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)).

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)).

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)).

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)).

test(phcs): Check for unused variables in PHPCS it self

2f2d1f1

klausi changed the title ~~test(phcs): Check for unused variables in PHPCS it self~~ test(phcs): Check for unused variables in PHPCS itself Apr 13, 2024

klausi added 2 commits April 14, 2024 11:43

Manually fixed all unused varaibles

267b0b9

Revert Slevomat dependency

4b9045c

klausi commented Apr 14, 2024

View reviewed changes

src/Filters/Filter.php Show resolved Hide resolved

klausi commented Apr 14, 2024

View reviewed changes

src/Files/File.php Outdated Show resolved Hide resolved

klausi marked this pull request as ready for review April 14, 2024 09:54

klausi changed the title ~~test(phcs): Check for unused variables in PHPCS itself~~ test(phpcs): Check for unused variables in PHPCS itself Apr 14, 2024

github-actions bot added Core Component: Config & Ruleset & CLI options Core Component: File Core Component: Reports Core Component: Tokenizer Core Component: Utils Focus: Comments/Docblocks Standard: Generic Standard: PEAR Standard: Squiz Status: triage labels Apr 14, 2024

jrfnl added Status: awaiting feedback Type: chores/QA and removed Core Component: Config & Ruleset & CLI options Core Component: File Core Component: Reports Core Component: Tokenizer Core Component: Utils Focus: Comments/Docblocks Standard: Generic labels Apr 15, 2024

jrfnl reviewed Apr 21, 2024

View reviewed changes

jrfnl added Status: awaiting feedback and removed Status: triage labels Apr 21, 2024

jrfnl mentioned this pull request May 2, 2024

Generic/DisallowYodaConditions: improve code coverage #465

Merged

11 tasks

github-actions bot added the Status: has merge conflict label May 15, 2024

klausi added 2 commits May 31, 2024 12:12

Merge remote-tracking branch 'origin/master' into check-unused-vars

18d445a

revert changes that should stay

507a3e4

github-actions bot removed the Status: has merge conflict label May 31, 2024

jrfnl removed the Status: awaiting feedback label Jun 2, 2024

jrfnl mentioned this pull request Jun 5, 2024

QA: review stray variables after loops #521

Open

jrfnl added this to the 3.10.x Next milestone Jun 5, 2024

jrfnl approved these changes Jun 5, 2024

View reviewed changes

jrfnl merged commit d140424 into PHPCSStandards:master Jun 5, 2024
40 checks passed

jrfnl added a commit that referenced this pull request Jun 5, 2024

Common::getSniffCode(): add tests

c75c7d2

Add initial set of tests for the `Common::getSniffCode()` method. Related to 146 Related to [review comment in PR 446](#446 (comment)).

jrfnl mentioned this pull request Jun 5, 2024

Documentation: improve information about Report data format + remove unused foreach keys #523

Merged

jrfnl mentioned this pull request Jun 5, 2024

Common::getSniffCode(): add tests, more defensive coding and minor simplification #524

Merged

jrfnl added a commit that referenced this pull request Sep 18, 2024

Common::getSniffCode(): add tests

fb6c2d5

Add initial set of tests for the `Common::getSniffCode()` method. Related to 146 Related to [review comment in PR 446](#446 (comment)).

jrfnl added a commit that referenced this pull request Sep 29, 2024

Common::getSniffCode(): add tests

9b837dd

Add initial set of tests for the `Common::getSniffCode()` method. Related to 146 Related to [review comment in PR 446](#446 (comment)).

jrfnl added a commit that referenced this pull request Sep 30, 2024

Common::getSniffCode(): add tests

70a5ae5

Add initial set of tests for the `Common::getSniffCode()` method. Related to 146 Related to [review comment in PR 446](#446 (comment)).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

klausi commented Apr 13, 2024

jrfnl commented Apr 13, 2024

klausi commented Apr 14, 2024 •

edited

Loading

jrfnl commented Apr 14, 2024

jrfnl left a comment

jrfnl commented May 16, 2024

klausi commented May 16, 2024

jrfnl commented May 16, 2024

klausi commented May 31, 2024

jrfnl left a comment

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

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

Conversation

klausi commented Apr 13, 2024

Description

Suggested changelog entry

Related issues/external references

Types of changes

PR checklist

jrfnl commented Apr 13, 2024

klausi commented Apr 14, 2024 • edited Loading

jrfnl commented Apr 14, 2024

jrfnl left a comment

Choose a reason for hiding this comment

jrfnl commented May 16, 2024

klausi commented May 16, 2024

jrfnl commented May 16, 2024

klausi commented May 31, 2024

jrfnl left a comment

Choose a reason for hiding this comment

klausi commented Apr 14, 2024 •

edited

Loading