Attribute duplicate-code messages to involved modules, not the last-checked one#10880
Attribute duplicate-code messages to involved modules, not the last-checked one#10880Pierre-Sassoulas wants to merge 5 commits intomainfrom
Conversation
Pierre-Sassoulas
added
Bug 🪲
duplicate-code
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10880 +/- ##
=======================================
Coverage 96.04% 96.04%
=======================================
Files 177 177
Lines 19625 19633 +8
=======================================
+ Hits 18848 18856 +8
Misses 777 777
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
force-pushed
the
fix-2368
branch
from
8d79dac to
6d8df7e
Compare
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 6d8df7e |
| total = sum(len(lineset) for lineset in self.linesets) | ||
| duplicated = 0 | ||
| stats = self.linter.stats | ||
| original_name = self.linter.current_name |
There was a problem hiding this comment.
Shouldn't this be current_ instead of original_?
There was a problem hiding this comment.
This is the name of the first file, current would make more sense if we were taking the file in the for loop that follows, no ?
There was a problem hiding this comment.
Is it the first file? I thought it was the last file to be linted and therefore the "current" file according to the PyLinter class
|
|
||
| # Attribute the message to the first involved module rather than | ||
| # the last-checked module which may be unrelated (see #2368). | ||
| first_module = min(c[0].name for c in couples) |
There was a problem hiding this comment.
What does the min do here?
There was a problem hiding this comment.
Make sure we're getting the module with the lower alphabetical value, so the result is deterministic. We're only raising one message per couples.
There was a problem hiding this comment.
Perhaps add this in the comment?
# Attribute the message to the first involved module rather than
# the last-checked module which may be unrelated (see #2368).
Does not really convey this to me
| first_module = min(c[0].name for c in couples) | ||
| self.linter.current_name = first_module | ||
| self.linter.current_file = self._module_filepaths.get(first_module) | ||
| self.add_message("R0801", args=(len(couples), "\n".join(msg))) |
There was a problem hiding this comment.
This really feels like we are misusing the linter here. Should emitting a message in a different module than the current one be an API we expose?
There was a problem hiding this comment.
We have a duplicate code to raise in another file, we have cyclic-import that behave like this too. I'm not sure there's any other way
There was a problem hiding this comment.
Might still be good enough reason to make this an actual (internal only) API rather than "patching" the current file like this?
There was a problem hiding this comment.
How about adding optional params to add_message as in self.add_message("R0801", args=(...), module="foo", filepath="foo.py") ? Not sure if we need to be protective of this API, this is a "normal use case", being able to lint multiple files is a distinguishing factor of pylint.
There was a problem hiding this comment.
Makes a lot of sense to me!
There was a problem hiding this comment.
Ddi it in #10894, we won't be able to backport this if we need a new API for a clean fix.
Pierre-Sassoulas
force-pushed
the
fix-2368
branch
from
6d8df7e to
12b0468
Compare
Refs #10880 — allows checkers to override the reported module name and file path in message location, instead of relying on the current file context or node. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…hecked one R0801 messages emitted in SimilaritiesChecker.close() were attributed to whichever module happened to be checked last, because add_message() without a node falls back to linter.current_name. Now the checker saves a module→filepath mapping during process_module() and sets the correct module context before each add_message() call. Closes #2368 Co-Authored-By: Claude Opus 4.6 <[email protected]>
Pierre-Sassoulas
force-pushed
the
fix-2368
branch
from
12b0468 to
8a02431
Compare
Refs #10880 — allows checkers to override the reported module name and file path in message location, instead of relying on the current file context or node. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Covers the new `module` and `filepath` parameters that allow overriding the reported message location. Refs #10880 Co-Authored-By: Claude Opus 4.6 <[email protected]>
2 participants
Type of Changes
Description
R0801 messages emitted in SimilaritiesChecker.close() were attributed
to whichever module happened to be checked last, because add_message()
without a node falls back to linter.current_name. Now the checker
saves a module to filepath mapping during process_module() and sets the
correct module context before each add_message() call.
Closes #2368
Co-Authored-By: Claude Opus 4.6 [email protected]