[3.x] Generic/LineLength: add ability to ignore long comment lines completely #1510
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
jrfnl
force-pushed
the
feature/3.x-line-length-ignore-comments
branch
from
7d951dc
to
9200521
Compare
As the sniff uses the `T_OPEN_TAG` token and then walks through the whole file in one go, the properties need to be changed in the test file **_before_** the open tag is encountered. In practice, this means that the unit tests for the changed properties need to be in separate files.
As suggested & discussed on twitter: https://twitter.com/MrDanack/status/874631725036523520
jrfnl
force-pushed
the
feature/3.x-line-length-ignore-comments
branch
from
9200521
to
ca96ed3
Compare
|
Thanks a lot for contributing this. Docs updated here: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties#genericfileslinelength |
|
@gsherwood Thanks for merging this! |
|
This feature was added incorrectly. Instead of ignoring lines that only contain comments, using the new Issue found when reviewing #2533 |
|
Sorry about that. Clearly needed more test cases. |
|
I found the older code confusing in the sniff, so easy mistake to make. I missed it during the review as well - looked fine. |
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.
Adds the ability to set a
$ignoreCommentsproperty to turn off all notifications about long comment lines.As suggested & discussed on twitter: https://twitter.com/MrDanack/status/874631725036523520
I've chosen a property-based implementation so as not to break BC for people who may have turned off the warning for long lines (so they only receive the error when the absolute limit has been reached).
If this would be implemented using a different errorcode for comments, these people who now start to see warnings about comments lines where they previously would not.
This property based implementation prevents that.
The property defaults to
falsewhich maintains the original behaviour of the sniff.Additionally, when adding the appropriate unit tests, I realized that the other custom properties for this sniff were not yet unit tested, so I've included a separate commit to fix that.
As the sniff uses the
T_OPEN_TAGtoken and then walks through the whole file in one go, the properties need to be changed in the test file before the open tag is encountered.In practice, this means that the unit tests for the changed properties need to be in separate files.