Disable D203, D212, D213 and S101 by default #2292
Conversation
not-my-profile
force-pushed
the
nursery
branch
from
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
not-my-profile
force-pushed
the
nursery
branch
from
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.
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.
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.
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.
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.
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.
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.
not-my-profile
force-pushed
the
nursery
branch
from
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.
not-my-profile
force-pushed
the
nursery
branch
from
0ff0972
to
f0e5b7f
Compare
not-my-profile
changed the title
|
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)