-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for NOLINTBEGIN/END #213
Conversation
0e8ef54
to
208cb3e
Compare
b683f2b
to
2cb090b
Compare
Generally thanks for the contribution and the effort you have put into this. Sorry that I am not more reactive as a maintainer. I see this feature was requested multiple times, so it would be a welcome addition. However there is a bit of devil in the details for how to handle multiple categories, I have added some comments. Possibly by not allowing nested blocks or multiple categories for now, most use-cases could be covered, and the change would be much simpler. I understand that it's frustrating to make a feature more restrictive if it seems so easy to make it very powerful, but keeping changes to cpplint simple is also important. |
I think one of those two features needs to be kept to make this feature usable. If blocks are large enough not being able to nest blocks or specify multiple specific categories would make this harder to use. I am going to try to simplify the nested handling by not allowing a category to be ignored multiple times. But if you really want to remove one of the features which one would you prefer to keep? |
So my personal preference would be to not have nesting, but allow multiple categories. But my opinion is no more valuable than yours, I do not actively use cpplint anymore, so I discuss without any personal stake in the matter as a user. I do have a stake as a maintainer, as all complexity leads to bugs and user confusion. My personal reasoning is that users of cpplint would generally like their source files to be 100% compliant with the global cpplint rules, but there can be some sections where multiple lines need to be exempt. That should remain a very rare situation, not a common one. And I would expect there to be a specific reason why one specific code block is exempt from following all global rules because of what this code block does. Such blocks should be kept rare and sparse. So overlaps of some categories should be very rare, and solvable by making each block wrapped individually. That way adding more code between the block does not make the in-between code accidentally be covered by some rule that bleed over. If two code blocks with some shared cpplint exclusions happen to follow each other in the code, I believe it is completely fine to have them redeclare their exclusions:
The fact that The only viable reason for wanting "nesting" I could see is if some project had different kinds of files which need different categories than the global one, but also need the ability to have special rules for block. In that case, I would prefer a separate mechanism to change the categories for a filetype outside of the file, as the reason would pertain to the semantic file type, and not specific code lines. Also I would prefer for closing |
c913538
to
dd2c31c
Compare
Add support for specifying a NOLINT in a block of code. The block is started with a NOLINTBEGIN comment which can specify one or more categories. The block is ended with a NOLINTEND comment which clears all categories specified in the BEGIN. Errors are generated if a BEGIN statment occurs in a block, if END is specified without a BEGIN or if END specifies categories This is similar to the clang-tidy NOLINTBEGIN/END feature.
dd2c31c
to
4a613c0
Compare
I rewrote the PR to not really support nested suppression blocks and only support an end block which stops suppression of all categories. |
8eaa2b8
to
8bd5461
Compare
cpplint.py
Outdated
|
||
def __init__(self): | ||
self._suppressions = {} | ||
self._open_block_suppressions = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to keep track of many open block suppressions, if only one block can be open at a time? (sorry I have not finished reading all the code, maybe later things would become clear). Currently it seem to me we don't need to track this at all, we can scan all suppressions any time to see if the last is open or not? The state of the runtime would become easier to reason about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way because it lined up with how single line ignores were being done. Also, in theory there could be some checks which are done outside of the line by line checks such as include what you use which could be eligible for suppression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @n3world, it's been a long time! Could you elaborate on why this would be needed in such cases? Also, I don't see where single-line ignores used such things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might have been some confusion in this comment thread. I think I originally thought the suggestion was to get rid of both _suppressions
and _open_block_suppressions
but now that I am rereading it I think the suggestion was just to remove _open_block_suppressions
. The list of open_block_suppressions is not actually needed and can just be the current open range. I think we still want to keep of the current open range because it makes it easier on end of block to close all open ranges. Making that change now
c2803c8
to
371329b
Compare
I'll be reviewing this soon. Next time, please don't run linters on the tests! They cause many merge conflicts |
Naming (and f-string) issues. |
@@ -6447,6 +6533,9 @@ def ProcessFileData(filename, file_extension, lines, error, | |||
include_state, function_state, nesting_state, error, | |||
extra_check_functions) | |||
FlagCxxHeaders(filename, clean_lines, line, error) | |||
if _error_suppressions.HasOpenBlock(): | |||
error(filename, _error_suppressions.GetOpenBlockStart(), 'readability/nolint', 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a confidence level of 1 as it has legitimate usecases, such as as a NOLINTFILE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to add NOLINTFILE that should probably be a different tag. Since this is linting for cpp which has explicit begin and ends to blocks I would argue that the end block should be required just like a }
is required for blocks in cpp code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO that would just add another extra chore. } is just one character, while NOLINTEND is a lot more. Anyways this is not really a blocker as I also plan to reevaluate verbosities when I get the time, but I really need to know about #213 (comment) before this gets merged.
Also, I'd appreciate it if you fixed the lint errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my review and reply above.
BTW the test changes were separate commits on the branch it would have been easier to just pop the commits off if they were a problem |
* Only track one open range * Rename utility function to ProcessCategory * Add other python builtin class methods to method allow list
Add support for specifying a NOLINT in a block of code. The
block is started with a NOLINTBEGIN comment which can specify
one or more categories. The block is ended with a NOLINTEND
comment which clears all categories specified in the BEGIN.
Errors are generated if a BEGIN statment occurs in a block, if
END is specified without a BEGIN or if END specifies categories
This is similar to the clang-tidy NOLINTBEGIN/END feature.
google#31
#137