-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[trailing-comma-tuple] Fix enabling with message control locally when disabled for the file #9609
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
Changes from all commits
171e40f
865d57c
1eedcbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -336,6 +336,7 @@ testoptions | |
tmp | ||
tokencheckers | ||
tokeninfo | ||
tokenization | ||
tokenize | ||
tokenizer | ||
toml | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
``trailing-comma-tuple`` should now be correctly emitted when it was disabled globally | ||
but enabled via local message control, after removal of an over-optimisation. | ||
|
||
Refs #9608. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
trailing-comma-tuple:3:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:4:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:5:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:6:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:31:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:34:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:38:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:41:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:47:0:None:None::Disallow trailing comma tuple:UNDEFINED | ||
trailing-comma-tuple:3:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:4:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:5:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:6:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:31:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:34:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:38:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:41:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:47:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:54:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:60:0:None:None::Disallow trailing comma tuple:HIGH |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
"""Check trailing comma tuple optimization.""" | ||
# pylint: disable=missing-docstring | ||
|
||
AAA = 1, | ||
BBB = "aaaa", | ||
CCC="aaa", | ||
FFF=['f'], | ||
|
||
def some_func(first, second): | ||
if first: | ||
return first, | ||
if second: | ||
return (first, second,) | ||
return first, second, | ||
|
||
#pylint:enable = trailing-comma-tuple | ||
AAA = 1, # [trailing-comma-tuple] | ||
BBB = "aaaa", # [trailing-comma-tuple] | ||
# pylint: disable=trailing-comma-tuple | ||
CCC="aaa", | ||
III = some_func(0, | ||
0), | ||
# pylint: enable=trailing-comma-tuple | ||
FFF=['f'], # [trailing-comma-tuple] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
[MAIN] | ||
disable=trailing-comma-tuple | ||
|
||
[testoptions] | ||
exclude_from_minimal_messages_config=True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got very confused by that error at first (Didn't see it was a specific automated test that was failing). I chose the fast fix but maybe taking into account the conf file after disabling everything and checking what's expected in the functional test would be better than the current behavior. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
trailing-comma-tuple:17:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:18:0:None:None::Disallow trailing comma tuple:HIGH | ||
trailing-comma-tuple:24:0:None:None::Disallow trailing comma tuple:HIGH |
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.
What do you think about factoring this out into a helper method? It's almost 20 lines with comments.
Uh oh!
There was an error while loading. Please reload this page.
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 do think we need to make this kind of optimization where we do not compute things when a message is disabled easier to use because the faster computation is the one you never do.
If we create an helper then we'd probably want to parametrize "trailing-comma-tuple" / "R1707" so it can be useful elsewhere with other messages (then we should be using the message store to get old name of the message if a message was ever renamed ?). And it adds a call.
We have the "perfect function" for this already with
self.linter.is_message_enabled(
applied on a line (barring some optimization to do here related to not launching it on each token as they are multiple token per line). It felt very heavy handed in this case so I did something very specific without check for old name / and without exact check of enabling or not.To be honest I made a lot of assumptions without ever doing a benchmark. One way to keep this clean and dry would be to make the message store better for this use case (token checks, and probably enabled but not absolutely sure it still is?).
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.
No worries, we can merge as is. I wasn't suggesting reusing it elsewhere, I just meant hoisting it out of this current location into a few lines up in the same file.