Generic/AssignmentInCondition: improve sniff code coverage #163
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rodrigoprimo
force-pushed
the
update-assignement-in-condition-code-coverage
branch
from
8ac9a3e
to
225c9cf
Compare
jrfnl
changed the title
jrfnl
reviewed
Dec 13, 2023
There was a problem hiding this comment.
LGTM with only one small remark. Will merge it once that's been fixed up. (Feel free to just amend the existing commit) Thanks @rodrigoprimo !
src/Standards/Generic/Tests/CodeAnalysis/AssignmentInConditionUnitTest.php
Outdated
Show resolved
Hide resolved
jrfnl
added
Status: awaiting feedback
Standard: Generic
and removed
Status: triage
labels
Doing this to be able to add more test case files to cover defensive code in the AssignmentInCondition sniff that protects it when checking code with parse errors.
This commit improves the test coverage for the AssignmentInCondition sniff by adding a few more test case files to exercise the parts of the sniff code that checks for invalid syntax.
rodrigoprimo
force-pushed
the
update-assignement-in-condition-code-coverage
branch
from
225c9cf
to
1fad6ab
Compare
|
Thanks for reviewing this PR, @jrfnl. I implemented the change you requested and I believe it is now ready to be merged. |
rodrigoprimo
added a commit
to rodrigoprimo/PHP_CodeSniffer
that referenced
this pull request
Dec 15, 2023
Replacing the default value with an empty string. Per Juliette's comment (PHPCSStandards#163 (comment)) there are three reasons why we want to do that: - The default value has no value in practice. It is an optional argument which is not enforced via the abstract functions, but is passed in all cases, so the default is never used in practice. - Setting the default value as file 1 also has an assumption implied, while IMO assumptions have no place in a test suite. - Maintainability - one less thing to have to keep in sync and to guard against typos
rodrigoprimo
added a commit
to rodrigoprimo/PHP_CodeSniffer
that referenced
this pull request
Dec 15, 2023
Replacing the default value with an empty string. Per Juliette's comment (PHPCSStandards#163 (comment)) there are three reasons why we want to do that: - The default value has no value in practice. It is an optional argument which is not enforced via the abstract functions, but is passed in all cases, so the default is never used in practice. - Setting the default value as file 1 also has an assumption implied, while IMO assumptions have no place in a test suite. - Maintainability - one less thing to have to keep in sync and to guard against typos
11 tasks
rodrigoprimo
deleted the
update-assignement-in-condition-code-coverage
branch
jrfnl
pushed a commit
that referenced
this pull request
Dec 15, 2023
Replacing the default value with an empty string. Per Juliette's comment (#163 (comment)) there are three reasons why we want to do that: - The default value has no value in practice. It is an optional argument which is not enforced via the abstract functions, but is passed in all cases, so the default is never used in practice. - Setting the default value as file 1 also has an assumption implied, while IMO assumptions have no place in a test suite. - Maintainability - one less thing to have to keep in sync and to guard against typos
jrfnl
pushed a commit
that referenced
this pull request
Dec 15, 2023
Replacing the default value with an empty string. Per Juliette's comment (#163 (comment)) there are three reasons why we want to do that: - The default value has no value in practice. It is an optional argument which is not enforced via the abstract functions, but is passed in all cases, so the default is never used in practice. - Setting the default value as file 1 also has an assumption implied, while IMO assumptions have no place in a test suite. - Maintainability - one less thing to have to keep in sync and to guard against typos
dingo-d
pushed a commit
to dingo-d/PHP_CodeSniffer
that referenced
this pull request
Dec 23, 2023
Replacing the default value with an empty string. Per Juliette's comment (PHPCSStandards#163 (comment)) there are three reasons why we want to do that: - The default value has no value in practice. It is an optional argument which is not enforced via the abstract functions, but is passed in all cases, so the default is never used in practice. - Setting the default value as file 1 also has an assumption implied, while IMO assumptions have no place in a test suite. - Maintainability - one less thing to have to keep in sync and to guard against typos
dingo-d
pushed a commit
to dingo-d/PHP_CodeSniffer
that referenced
this pull request
Dec 23, 2023
Replacing the default value with an empty string. Per Juliette's comment (PHPCSStandards#163 (comment)) there are three reasons why we want to do that: - The default value has no value in practice. It is an optional argument which is not enforced via the abstract functions, but is passed in all cases, so the default is never used in practice. - Setting the default value as file 1 also has an assumption implied, while IMO assumptions have no place in a test suite. - Maintainability - one less thing to have to keep in sync and to guard against typos
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR improves the code coverage for the AssignementInCondition sniff. This sniff has some defensive code to protect against parse errors, and those were not exercised by the tests. I followed the instructions on #143 for creating parse error tests.
I also checked if there are new assignment operators added in more recent versions of PHP that are not supported by this sniff, but I did not find anything.
Suggested changelog entry
Improve AssignementInCondition sniff code coverage
Related issues/external references
Part of #146
Types of changes
PR checklist