[Ruff 0.6] Stabilise PLR6104
#12858
Merged
[Ruff 0.6] Stabilise PLR6104
#12858
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
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLR6104 | 358 | 358 | 0 | 0 | 0 |
Linter (preview)
✅ ecosystem check detected no linter changes.
|
Yeah, i"m not entirely sure about this one either. I do think it improves readability in some cases because it makes the precedence more clear. But the fact that the rule has no fix (according to the ecosystem results) makes the adoption fairly painful. To me it's not entirely clear if I would consider this an idiomatic rule or a stylistic rule. If it's the latter, then our rule acceptance guidelines would say that we should not accept the rule. If it's idiomatic, then I would probably go ahead |
it seems like it does have an autofix, but that the autofix must be marked as unsafe in many situations :/ |
|
Having discussed this offline, we'll merge this since:
|
Merged
|
@AlexWaygood I'm a bit concerned about this rule, because there are quite some cases where the application would unintentionally introduce nasty and hard-to-find bugs, in particular when it is applied on input arguments of a function. I'm afraid one could sometimes not be aware that applying the fix causes bugs for these cases. It is especially prevalent for data science libraries when working with numpy arrays or pandas Series / DataFrames, but it could also happen in case the argument is a dictionary or list. In my view, these are false positives which could and should be avoided by not applying the rule on variables that are function arguments. Skimming through the examples above from the ecosystem, I found 10 such cases where applying the fix would lead to (not always evident) bugs:
Would it be possible to somehow avoid raising PLR6104 for variables that are function arguments? |
|
By chance, I think the first one I clicked on (pandas/core/arrays/boolean.py#L239) is not a bug? I opened a couple others and you look correct though. I think that's a fair point, it does seem especially dangerous to apply this rule to variables bound to function inputs. I'm not sure if we have logic based on that already, it seems feasible? What's the deal with this rule in Pylint? Does it have the same problems? i.e. does it report violations for the examples you selected? |
|
Thanks for speaking up @kdebrab, really appreciate it. I'll revert this PR tomorrow morning before we cut the release, so we can take our time to consider this more fully. |
AlexWaygood
added a commit
that referenced
this pull request
Aug 14, 2024
AlexWaygood
added a commit
that referenced
this pull request
Aug 14, 2024
|
Reverted in #12884 |
|
I've opened #12890 to discuss if we can reduce the number of unsafe recommendations from this rule. |
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.
Split out from #12857 so that it can be considered in isolation.
Summary
This PR stabilises the
non-augmented-assignmentpylint rule (PLR6104). I think this rule makes sense and is reasonably uncontroversial. It's been in preview for >3 months and there haven't been any significant issues opened about it in several months.However, it's a stylistic rule that's somewhat pedantic, and it has a lot of ecosystem hits.
Test Plan
cargo test