Repository navigation
Document rule acceptance guidelines - #27910
Conversation
| statements, like imports) or `analyze/expression.rs` (if your rule is based on analyzing | ||
| expressions, like function calls). | ||
|
|
||
| 1. Map the violation struct to a rule code in `crates/ruff_linter/src/codes.rs` (e.g., `B011`). |
There was a problem hiding this comment.
Can/should we drop this requirement yet? 😮
It feels like probably not, but it also seems awkward to have to choose both a category and a linter group. This macro also generates a ton of Rule code that we'll have to generate some other way if we stop requiring this mapping.
There was a problem hiding this comment.
I don't think we can today? We can remove it once our rule infrastructure supports it
There was a problem hiding this comment.
Yep, this is related to what I mentioned for my plan this week. I'm looking into what it would take to support this, but we definitely can't drop this from the docs yet.
| ### Example: Adding a new lint rule | ||
| ### Proposing a new lint rule | ||
|
|
||
| A proposal for a new lint rule in Ruff should include the following components: |
There was a problem hiding this comment.
These are a lot of requirements for proposing a new rule. I think i'd set this as a bar for a PR, or for a rule to be accepted. But I don't think we should enforce this when users open an issue with a rule idea. Even for an issue, that's probably more than I'd ask for.
I think, for an accepted rule, we should have a proposed name, a category, and a draft of What it does, why this is bad, and maybe an example.
There was a problem hiding this comment.
I think I agree with you that it's a lot to ask. We do get a lot of very brief rule requests that linger with
needs-decision
Maybe the name, category, and other sections you mention are a better sweet spot, though.
What do you think about the proposed diagnostic? I thought that was kind of a fun way to capture a lot about the proposed rule without requiring too much work.
I'd also like to find somewhere else to put all of these optional sections, if we remove them here. I don't think we currently have a canonical list of typical documentation sections. Maybe I could better clarify that all of these requirements should be met by the time a rule is added and note which parts can be deferred for the implementation PR. That would help with the ecosystem circularity too.
I guess we don't want to discourage too many people from opening rule requests, but I kind of don't mind documenting the "ideal" process, assuming that most people won't read or follow it anyway.
I added a note trying to clarify this for now.
There was a problem hiding this comment.
I agree with you on needs-decision and I'd love to reduce the number of issues labeled with needs-decision. What if it's more a two-stage process. You can still open issues with very brief descriptions. We take an initial look to decide whether this roughly fits into Ruff and, if so, mark it as needs-design and ask for someone to fill out our template (we can link to this very specific guide). Requests that never make it past needs-design can be closed after , unless we think this is a rule we should add.
What do you think about the proposed diagnostic? I thought that was kind of a fun way to capture a lot about the proposed rule without requiring too much work.
I think it's a very nice idea because it also raises questions around the rule's span and how to convey the problem. But I don't think I'd make it a requirement for a rule to be accepted. It's a nice bonus. It's something that we can also iterate on in the PR.
There was a problem hiding this comment.
I added an initial note about a brief issue being okay, mentioning the needs-design step, and further separating out the parts needed for the design proposal and implementation.
| almost always a mistake, but in some narrow cases beyond Ruff's understanding, it may be | ||
| intentional, in which case a `noqa` or `ruff: ignore` comment should be used. Such suppression | ||
| comments should essentially never be reasonable for a `correctness` lint but are fine for | ||
| `suspicious` lints. | ||
|
|
||
| At a high level, the steps involved in adding a new lint rule are as follows: | ||
| The final branch of the flow chart deals with stylistic lints, which are again somewhat subjective | ||
| to differentiate between because changes that make code simpler often also make the code faster and | ||
| more idiomatic. Thus, the question prompts you to consider the _primary_ improvement. Rules that | ||
| primarily make code simpler are `complexity` lints, those that primarily make code faster or use | ||
| less memory are `performance` lints, and those that primarily make code more idiomatic are `style`. | ||
|
|
||
| 1. Determine a name for the new rule as per our [rule naming convention](#rule-naming-convention) | ||
| (e.g., `AssertFalse`, as in, "allow `assert False`"). | ||
| #### Other guidelines | ||
|
|
||
| Following these steps should generally ensure that a rule is a good fit for Ruff. A couple of | ||
| additional things to watch out for are: | ||
|
|
||
| - Rules that conflict with other tools, or especially other rules | ||
|
|
||
| Although we have many existing `formatting` rules that overlap and even conflict with our | ||
| formatter, we are not eager to add more. Similarly, we should avoid rules that mainly support | ||
| type checker usage, when type checkers themselves emit similar diagnostics. Most clearly, we | ||
| should avoid rules that overlap or conflict with other lint rules. This often suggests that the | ||
| existing rule should instead be made configurable to toggle between the two behaviors. | ||
|
|
||
| Checking both the input and output examples from your rule proposal with `ALL` rules selected in |
There was a problem hiding this comment.
We could link to multiplay here. @AlexWaygood do we have a public link?
There was a problem hiding this comment.
multiplay is a public repo! https://github.com/astral-sh/multiplay
multiplay is still not hosted anywhere, so you do have to clone the repo and then run it locally, though
There was a problem hiding this comment.
I thought about adding pre-configured Ruff and ty playground links here at least. I'll give that a try since multiplay isn't hosted anywhere.
| libraries, but this niche should be filled by plugins in the near future. Most Ruff rules should | ||
| apply to Python language features or to functionality from the standard library. |
There was a problem hiding this comment.
Clippy explicitly is open to widely used third party libraries. I do think this makes sense, similar to ty supporting attrs etc. But I agree, I wouldn't count airflow as one such library because it's very specific. But libraries that are so widely used that you find them in almost every application, having suspicious and correctness rules for those seems valuable to most of our users.
I'd feel more hesitant from adding deprecation rules for third party libraries. That feels like a good use for plugins
There was a problem hiding this comment.
I agree with all of that. Does this read as too firm? I tried to make this whole #### Other guidelines section read as general guidance with room for exceptions, but that may not come across clearly enough.
I intended it to be somewhat firm but may have gone too far.
There was a problem hiding this comment.
It does sound to me that rule requests for third-party libraries would always be reject. Our stance here might also change once we support plugins
There was a problem hiding this comment.
I've tried to soften this and also removed the mention of plugins for now.
96dc01c to
bae1566
Compare
bae1566 to
268fbc2
Compare
a545fa3 to
62615d4
Compare
|
62615d4 to
6cf340f
Compare
|
|
||
| #### Other guidelines | ||
|
|
||
| Following these steps should generally ensure that a rule is a good fit for Ruff. A couple of |
There was a problem hiding this comment.
I actually don't see much in here that explains when Ruff would accept a rule, it's much more about the mechanics of how to add one. What criteria would you (the Ruff maintainers) use to decide whether or not to accept a rule? This guideline offers some special cases but not a general set of principles on which to base the decision.
There was a problem hiding this comment.
I can see what you mean, but I guess my thinking was if you've followed these steps and come up with:
- a good name
- a suitable category
- convincing "what it does" and especially "why is this bad" docs
and not violated any of these special cases (or made a case for why your rule is an exception), then we'd accept the rule. I don't think the goal is to set a super high bar, just standardize some of the things we look for and make sure we have enough information for an acceptance decision. That's part of why I didn't actually end up using "Rule Acceptance Guidelines" directly in the docs.
One of the goals of the new categories is actually to make it easier to add rules without having to worry so much about their linter codes.
6cf340f to
8d745ad
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
8d745ad to
58a4878
Compare
0a21c1b to
eb3e945
Compare
| Note that "useless" in the context of a `correctness` rule should be indicative of a mistake. For | ||
| example, `useless-exception-statement` (`PLW0133`) flags exceptions that are constructed and never | ||
| raised, suggesting that the user forgot to raise or otherwise use them. In contrast, code that is | ||
| merely unnecessary but still working as intended may be a better fit for `complexity`, such as | ||
| `useless-try-except` (`TRY203`). As described above, if the code is probably not intentional but may | ||
| be in certain cases, `suspicious` can also be a good fit. |
There was a problem hiding this comment.
Added this note about "useless" vs "unnecessary" in light of our discussions.
There was a problem hiding this comment.
Thanks. I like this. I wonder if we should also mention style.
eb3e945 to
79d0963
Compare
79d0963 to
b3e39de
Compare
b3e39de to
81f28bb
Compare
Summary -- This PR includes the contributor-facing documentation for the new categories, as well as updated rule acceptance guidelines built atop them. Test Plan -- Future rule requests and manual testing of the rule request template in my personal Ruff fork
Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Micha Reiser <micha@reiser.io>
81f28bb to
e20dedb
Compare
Summary -- This PR includes the contributor-facing documentation for the new categories, as well as updated rule acceptance guidelines built atop them. I think we could also update our rule request template based on some of these guidelines, but I'll save that for a follow-up once these are solidified. Test Plan -- Future rule requests --------- Co-authored-by: Micha Reiser <micha@reiser.io>
Summary
This PR includes the contributor-facing documentation for the new categories, as well as updated
rule acceptance guidelines built atop them.
I think we could also update our rule request template based on some of these guidelines, but I'll save that for a follow-up once these are solidified.
Test Plan
Future rule requests