More thorough documentation for rules

The idea is that for each rule we want to document:

what exactly the rule is (in case it isn't obvious)
what the reasoning behind the rule is (in case it's not obvious)
if the autofix always works or only sometimes works (and in that case explain when it doesn't work)
an example of a true positive
an example of a false positive (if applicable)
an example of a false negative (if applicable)

How existing linters document their lints:

clippy: https://rust-lang.github.io/rust-clippy/
ESLint: https://eslint.org/docs/latest/rules/

Original description

Currently checks are only documented via their error messages, which can be quite uninformative. For example:

def foo(text):
    return [l for l in text.splitlines() if not l.startswith('#')]  # E741 Ambiguous variable name: `l`

This error message does not tell us what the check actually is, e.g. is any single letter variable name considered ambiguous? Looking up the pycodestyle documentation requires three clicks starting from the README of ruff, followed by a Ctrl+F only to find out:

E741: do not use variables named ‘l’, ‘O’, or ‘I’

But even the pycodestyle documentation does not document the reasoning behind this check, ... you have to look up PEP 8 to find:

In some fonts, these characters are indistinguishable from the numerals one and zero. When tempted to use ‘l’, use ‘L’ instead.

Which would be enough information for me to just disable that check because any self-respecting programmer uses a font that clearly distinguishes these characters.

Two more examples:

TID252: Relative imports are banned ... why would you want to ban relative imports? (I honestly don't know but I would like to know ...)
The reasoning for D200: One-line docstring should fit on one line would be https://peps.python.org/pep-0257/#one-line-docstrings

In particular in editor popups shown by ruff-lsp I think every error message should explain:

what exactly the check is (in case it's not obvious)
what the reasoning behind the check is (in case it's not obvious)

The first is necessary if you want to fix the check and the second is necessary if you want to make an educated decision about disabling the check, in case you don't actually need it.

We should figure out a structured format for this, and include it as a macro or via rustdoc so that it can be picked up by tooling, similar to what we did with #[option(...)].

Related: #654. (And, to an extent: #1398.)

I want to get an mdBook going, then figure out how to incorporate the check documentation into that artifact.

I want to note that Clippy has its own web app to explore lints that is simply implemented in JavaScript and loads the lints from a .json file.

I think this is a nice approach since it can be much more easily customized with advanced interactivity / features than mdbook, so I think something like that would also make sense for the rules of ruff.

See also an example of how clippy documents lints. It's just Markdown in doc comments with standardized sections. I think that should also work well on the structs introduced in #1685.

This makes sense. Following Clippy is a good call.

I know this is a big ask, and we would need a frontend person to volunteer. But having a slick page like rocket would be super helpful. It could explain the benefits of using ruff in pre-commit, in IDE's and CI. With savings, in time, money and energy. Then just have some good docs on all the lints.

I'd focus on good docs first ... once we have those it should be very easy to embed them anywhere. But yeah I agree an overall website would make sense especially since we already have an online playground.

100%. I just want a page where people could input some information and get an estimate of how much time, money, and the environmental impact of a conversion. Of course very low priority, but well into the future would be dope.

Suggested addition to the list: which configurations affect it. I think it is a useful metadata to have on a rule. (Bonus points if this metadata is enforced as being correct, i.e. a rule implementation can't access a configuration that it hasn't declared it's using).

Maybe the documentation of my flake8-plugin could also suit ruff well: https://github.com/MartinThoma/flake8-simplify#sim102

Each rule has:

A code block: bad vs good (an example what triggers the rule vs how it should be)
An anchor - this way people can directly link to single rules
The title of the rule links to the issue in which the rule was introduced (see https://github.com/MartinThoma/flake8-simplify#rules ). That contains:
- An explanation why this rule makes sense, e.g. [SIM117] Merge with statements for context managers that have same scope MartinThoma/flake8-simplify#35
- A discussion about potential issues

I like the code block + links a lot as this makes it searchable and easy to understand without reading too much. I like having an anchor to single rules as people can lead others to the documentation. I like having all rules on one page (in a compact format + preferably only the rules) as it allows me to just Ctrl+F for a specific rule (e.g. after --select=ALL --fix and seeing a change I don't like).

I've found the lack of documentation on rule violations as quite a sore point about using Ruff.

While trying to look for explanations for some of my rule violations I came across the supported repo Tryceratops which had very easy to read documentation of their exceptions rules. I know the organization format will not work in Ruff due to the sheer lack of rules Ruff supports in comparison.

However the style structure of their docs is something I would appreciate in Ruff as well.

This now exists in a basic form. We can define the docs as part of the define_violation! macro, identically to Clippy:

define_violation!(
    /// ### What it does
    /// Checks for `self.assertRaises(Exception)`.
    ///
    /// ## Why is this bad?
    /// `assertRaises(Exception)` can lead to your test passing even if the
    /// code being tested is never executed due to a typo.
    ///
    /// Either assert for a more specific exception (builtin or custom), use
    /// `assertRaisesRegex` or the context manager form of `assertRaises`.
    pub struct NoAssertRaisesException;
);

This explanation shows up with ruff rule B017:

❯ cargo run rule B017
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `target/debug/ruff rule B017`
[flake8-bugbear] assert-raises-exception (B017)

### What it does
Checks for `self.assertRaises(Exception)`.

### Why is this bad?
`assertRaises(Exception)` can lead to your test passing even if the
code being tested is never executed due to a typo.

Either assert for a more specific exception (builtin or custom), use
`assertRaisesRegex` or the context manager form of `assertRaises`.

We also generate these as standalone Markdown files, which are linked from the README and from the beta docs, to the following pages respectively:

Let's open new issues to track (1) any modifications or improvements we want to make to the doc structures themselves (this just follow Clippy), and (2) actually adding these docs, since right now, B017 is the only rule that has it.

Navigation Menu

Repository navigation

More thorough documentation for rules #1467

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

More thorough documentation for rules #1467

Description

Activity

charliermarsh commented on Dec 30, 2022

charliermarsh commented on Dec 31, 2022

not-my-profile commented on Jan 6, 2023

not-my-profile commented on Jan 7, 2023

charliermarsh commented on Jan 7, 2023

colin99d commented on Jan 14, 2023

not-my-profile commented on Jan 14, 2023

colin99d commented on Jan 14, 2023

bluetech commented on Jan 29, 2023

MartinThoma commented on Feb 2, 2023

nunokaeru commented on Feb 6, 2023

charliermarsh commented on Feb 7, 2023

charliermarsh commented on Feb 7, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions