[trailing-comma-tuple] Fix enabling with message control locally when disabled for the file

[Pierre-Sassoulas]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Refs #8606, #9620 (follow-up)
Closes #9608

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.84%. Comparing base (ce47a62) to head (1eedcbb).
Report is 162 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9609   +/-   ##
=======================================
  Coverage   95.84%   95.84%           
=======================================
  Files         174      174           
  Lines       18897    18904    +7     
=======================================
+ Hits        18112    18119    +7     
  Misses        785      785           

Files	Coverage Δ
pylint/checkers/refactoring/refactoring_checker.py	98.27% <100.00%> (+<0.01%)	⬆️
pylint/lint/message_state_handler.py	96.48% <ø> (ø)

... and 2 files with indirect coverage changes

[DanielNoord]

Just saw your comment on the issue. This does indeed not work.

[Pierre-Sassoulas]

Jeez, I wonder why ruff don't have fine grained message control yet 😄 (I did not benchmark this yet, I probably need to test some assumptions about the caching of per line enable vs checking the enable on each token).

[Pierre-Sassoulas]

Wow, is_message_enabled is seriously misleading. I suggest we rename to is_message_enabled_for_current_file or create an argument for the file which default to current for 3.0 and to None in 4.0, what do you think @DanielNoord ?

[DanielNoord]

Hmm, do we know if this actually meaningfully influences performance? Yes calling a function often is bad, but if the effect on performance is negligible than we might not want to overcomplicate our config handling (again).

[Pierre-Sassoulas]

I was thinking about misleadingness rather than performance. {Number of files} calls is not that problematic, but the function name making me lose 2 hours of debug is :D

[Pierre-Sassoulas]

By the way I still don't understand why locally everything work and not in the CI, but I'm short on time atm.

[nickdrozd]

Does this fix #8201?

[jacobtylerwalls]

Thanks for your work here on this important improvement. I'm wondering what you think about backporting performance improvements. Should we try to keep the patch releases more stable by only backporting bugfixes and not refactors?

[jacobtylerwalls]

Oh woah, I had it backwards. This is the bugfix. Carry on!

[Pierre-Sassoulas]

I can understand the confusion because I'm trying very hard to optimize the bug fix itself 😄

[Pierre-Sassoulas]

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.

Was a review on #9620, this MR was repurposed.

[jacobtylerwalls]

What do you think about factoring this out into a helper method? It's almost 20 lines with comments.

[Pierre-Sassoulas]

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

[jacobtylerwalls]

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.

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 1eedcbb