-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Disable D203, D212, D213 and S101 by default #2292
Conversation
e216392
to
6bcc198
Compare
D203 from pydocstyle conflicts with PEP 257[1] since its wording has been changed[2] and thus shouldn't be enabled by `--select D` or `--select ALL` D212 and D213 conflict with each other which can lead to ruff performing an infinite autofix iteration loop (Ruff goes back-and-forth, adding and removing a newline between the docstring and the class definition, until you hit 100 iterations). We display a warning, but it's just a bad experience, and it comes up way too often. Rules that have been disabled by default can still be explicitly enabled. `ruff explain` now mentions when rules have been disabled by default as well as the reason and the README table generation has also been updated accordingly. [1]: https://peps.python.org/pep-0257/ [2]: python/peps@eba2eac
6bcc198
to
1f69610
Compare
Does specifying |
Good question. Previously (Marking this as a draft till I implement that ... probably only have time for that in a couple of hours.) |
|
Rule::MagicValueComparison => { | ||
Some("Comparisons to strings are generally not considered to be an anti-pattern.") | ||
} | ||
Rule::AssertUsed => Some("`assert` is generally not considered to be an anti-pattern."), |
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'd like to remove this, Rule::MagicValueComparison
, and Rule::MultiLineSummaryFirstLine
from this list for now. We can always add them later. But I don't want to couple the debate here to the removal of specific rules, apart from those that motivated the issue. The goal here is really to solve the infinite iteration loop for new users.
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.
D212 (MultiLineSummaryFirstLine) is [part of a valid convention], and shouldn't be part of any sort of automatic disable list IMO
It is not part of PEP 257, which I'd consider to be the only default convention. I don't think it makes sense to enable convention-specific rules by default just because they are part of some convention ... there are many conventions and unless the user has configured convention
we don't know which convention they use. Case in point: pydocstyle also does not enable D212 by default.
/// The returned reason is documented in the autogenerated README as well as reported by the explain command. | ||
pub fn nursery_reason(rule: &Rule) -> Option<&'static str> { | ||
match rule { | ||
Rule::OneBlankLineBeforeClass => Some("Conflicts with PEP 257."), |
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.
Here's where I'm torn: assume that no user ever enabled this rule. Why support it? Why maintain the code? The tests? Etc.
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 am alright with dropping these rules completely. However you don't want to break configs that currently ignore these rules and since the RuleCodePrefix
serde impls are currently automatically derived, I think the easiest way to achieve that would be to introduce a no-op rule and map the legacy codes to that no-op violation that simply never gets triggered ... however since we don't yet have the many-to-many mapping we'd need to assign a rule code to the no-op rule ... which feels wrong since it's very much an implementation detail. So I'd rather have us wait with the dropping of these rules until after the many-to-many mapping has been implemented.
And I also think it would be nice to have some deprecation period with ruff still supporting the rules but printing a warning that we are planning on dropping them with a link to a GitHub issue about that, so that in case these rules are in fact used by some users they can voice their opinions about it.
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 is reasonable. I'd like to get these rules out of --select D
and --select ALL
as soon as we can. And the approach you've proposed here does achieve that (as long as we preserve retaining D212
when the appropriate conventions are set, as per the discussion above).
I do think it's strange to put these rules in nursery
rather than mark them as explicitly deprecated (and use the strategy you've implemented here to handle those deprecations), since we know we want to remove them. My preference would be to leave S101
and D212
for now, and use the strategy here for D203
and D213
, but mark them as deprecated rather than in nursery. To me, nursery suggests that there's a path to stabilizing these rules, but the intent here really is to discourage and stop supporting them.
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 am not convinced that keeping these rules around actually is much of a maintenance burden ... once the conflict / endless iteration issue has been fixed by disabling them by default they really shouldn't be such a hassle anymore ... but if you want to remove them that's very much your call.
1f69610
to
0ff0972
Compare
S101 flags every `assert` statement. `assert` statement aren't generally considered to be an anti-pattern, so this commit disables the rule by default.
0ff0972
to
f0e5b7f
Compare
Just an update:
|
Regarding the |
@not-my-profile - This came up again recently (#2368). What can we do to prioritize turning those I'd like to start by just disabling the |
This is another temporary fix for the problem described in #2289 and #2292. Rather than merely warning, we now disable the incompatible rules (in addition to the warning). I actually think this is quite a reasonable solution, but we can revisit later. I just can't bring myself to ship another release with autofix broken-by-default 😂
(see our discussion in #2289 as well as the commit messages for the reasoning)