astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
33.38k stars 1.11k forks source link

Clarify and document process/criteria for the inclusion of new checks #2257

Open sciyoshi opened 1 year ago

sciyoshi commented 1 year ago

People who are looking to adopt Ruff will invariably have differing needs from their linter. There are currently at least 400 flake8 plugins/linter packages on PyPI, and it is unlikely that Ruff will grow in scope to support all of them (nor should it, I think).

Given this, and especially in the absence of a plugin system, I think it would be helpful to document what checks would be accepted into Ruff. There is some information on the maintenance team's perspective on this scattered throughout comments in the GitHub issues. Perhaps the clearest stance I've seen is laid out here, but there are also some related thoughts here and here. If I were to attempt to summarize:

  1. There should be a clear rationale for why a check is useful.

    An obvious criteria, and I would suggest that such a rationale would make sense to include as part of any documentation for the check.

  2. It should be sufficiently robust, i.e. few false positives and negatives.

    For checks that are enabled by default in Ruff, I would agree with this. However, there is a class of checks that are useful to enable in some scenarios, but that shouldn't be enabled by default and should likely be explicitly opt-in. Would these be considered for inclusion? There's been discussion of having check categories, and maybe the answer is that the work there needs to be completed first.

  3. It should not be overly complex to implement relative to its usefulness.

    This might exclude some otherwise useful checks, but I think limiting scope in this way makes sense in order to reduce the maintenance burden.

  4. There should be evidence of demand.

    Does this mean the check is part of a flake8 plugin with N downloads/month for some N >> 0? Or is opening GitHub issue with a few votes enough to count?

I would suggest another criteria: checks should likely be library-agnostic. Note that some upstream checks that are planned for inclusion already go against this, e.g. Django- and requests-related checks in flake8-bandit.

As an example, ESLint documents their guidelines for new rules here. Including a similar section with some of the above information in CONTRIBUTING.md would I think be helpful.

There is also the broader question of whether Ruff is planning to take an opinionated stance on linting (akin to black for formatting), but maybe that is a separate discussion.

sciyoshi commented 1 year ago

Disclosure: I bring this question up because of a specific security-related check I'd be interested in seeing in Ruff that we've been using in our own codebase via a custom flake8 plugin, that is broadly useful but will likely have false positives (should not be enabled by default), and that has been requested in flake8-bugbear and flake8-bandit but not yet implemented. At the moment, it seems there isn't a great place to add it (short of creating a new flake8 plugin for it specifically), but if there is a chance of it being accepted I'd be happy to do that work.

charliermarsh commented 1 year ago

This is great and a much-needed discussion. I can't write a complete response right now but I'll try to return to this as soon as I can (hopefully this weekend).

sciyoshi commented 1 year ago

Two other thoughts/questions that came to mind along similar lines:

berzi commented 1 year ago

Additional question: is the process of implementing new checks documented for potential contributors? Upon a (very) cursory look I couldn't find anything and if I wanted to contribute I'd have to read existing code and try to emulate it, which might result in suboptimal implementation.

charliermarsh commented 1 year ago

@berzi - Yeah, the contributing section has some guidance on adding new rules! https://beta.ruff.rs/docs/contributing/#example-adding-a-new-lint-rule

jankatins commented 1 year ago

Another suggestion I would have is a "black"-like opinionated but more or less universally applicable set of checks as default: checks for clear bugs, (by some definition) sane formatting, typing problems, old style code which has newer versions, security issues, ... So I can drop in ruff . or even ruff . --fix and get a nice codebase (after fixing all checks) without having to configure anything at the start. Similar to the black experience: It makes sense, one might have a slightly different opinion but having one thing implemented makes the codebase better than not having ruff run on it. no matter the opinions.

berzi commented 1 year ago

@jankatins A default configuration already exists. One might argue it's currently too conservative, but that seems like a discussion for a separate issue.