astral-sh / ruff

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

Unified command for linting and formatting #8232

Open zanieb opened 1 year ago

zanieb commented 1 year ago

It should be possible to check if files would be formatted and if there are any lint violations in a single Ruff invocation.

It should be possible to apply linter fixes and formatting changes in a single Ruff invocation.

The design of this interface is still being discussed internally. This issue will be updated as development continues.

Previous discussion at https://github.com/astral-sh/ruff/discussions/7310#discussioncomment-7376397, https://github.com/astral-sh/ruff/discussions/7310#discussioncomment-7102010, and https://github.com/astral-sh/ruff/issues/7232

zanieb commented 1 year ago

Roughly, one proposed interface would be the generalization of ruff check and the addition of flags such as --lint / --no-lint and --format / --no-format to enable specific tools. These flags would also be configurable in the settings as ruff.check.lint and ruff.check.format (or ruff.lint.enabled and ruff.format.enabled).

On ruff check --format, unformatted files would be flagged as a violations (similar to lint rule violations). On ruff check --format --fix, unformatted files would be formatted.

Further configuration of each tool would be done from the respective ruff.lint and lint.format sections.

I believe, eventually, it would be ideal for --format (and any other future tools) to be enabled by default when running ruff check.

This approach would require significant consideration of our existing check diagnostic and violation design, as it is very lint focused at the moment. Consideration should be made for the possibility of future tools that would raise diagnostics, such as a type checker.

It's an open question whether a dedicated ruff lint command would be added.

T-256 commented 1 year ago

If you ask me about how to join into one command, my answer (and I think most of users) would be put formatter as a lint rule. Like https://github.com/astral-sh/ruff/discussions/7310#discussioncomment-7102104, in my opinion formatter actually is linter rule(s) with safe-autofix: ruff check . --select E,FMT --fix

@zanieb I'm pretty sure you also considered this, but I didn't find why you ignored this approach. Can you show what are blockers for this design? solving those blockers maybe less expensive for team (and users)?

For now, I think --add-noqa (and --watch?) won't work as desired on this approach.

MichaReiser commented 1 year ago

We discussed this internally. Building the formatter as a linter raised a couple of design questions:

Overall, formatting just felt different enough.

hmvp commented 1 year ago

We discussed this internally. Building the formatter as a linter raised a couple of design questions:

* The formatter would need to support `noqa` suppression comments

Isn't that the job of the linting mechanism that works based on whether the formatting check would find anything.. How does this work for I001 (isort)?

  • The CLI would be fundamentally different from how editors work where editors use different commands for formatting and linting.

I feel it makes sense to keep formatting also as a separate command for these use cases

  • How would we support enabling preview style formatting for the formatter without enabling preview behavior of other lint rules. Make it two different rules? You can even (if not configured explicit) enable the preview rule in preview and disable the normal formatting rule...

Overall, formatting just felt different enough. I kind of disagree. But if it is, import sorting also falls in this category...

T-256 commented 1 year ago

Thanks, I didn't realize these issues, here are my proposed solutions for them:

  • The formatter would need to support noqa suppression comments

A. Use #fmt: comments for formatting violations. B. We can warn noqa suppression is not available for this rule.

  • The CLI would be fundamentally different from how editors work where editors use different commands for formatting and linting.

For formatting, editors can keep current behavior, also for linting and autofixes I don't think FMT requires any change to output structure.

  • How would we support enabling preview style formatting for the formatter without enabling preview behavior of other lint rules.

Add format-preview and lint-preview. (--(no-)preview will apply for both linter and formatter)

zanieb commented 1 year ago

How does this work for I001 (isort)?

I kind of disagree. But if it is, import sorting also falls in this category...

We are also of the opinion that import sorting is not well suited to be a lint rule. We are still trying to figure out the best way to resolve that.

Add format-preview and lint-preview...

We already have this as ruff.lint.preview and ruff.format.preview. If the formatter is implemented as a lint rule then it would be confusing that it's unaffected by lint.preview though.

I was originally a strong proponent of using a lint code for the formatter. Here are some notes:

A new formatting lint category i.e. FMT. Formatter violations could be implemented as rules e.g.

  • FMT001 file would be formatted
  • FMT002 invalid placement of fmt pragma

Some thoughts:

  • Avoids collision with --format <style>
  • Simple integration with existing CLI
  • Does it make sense to have more than one format code?
  • How does this interact with existing linter noqa pragmas?
  • Can users ignore individual format violation codes?
  • Can we feasibly split formatting into discrete pieces?
  • Avoids adding new options to the CLI
  • Formatter violations are not special-cased
  • A file requiring formatting would not work well with our current diagnostic system
  • Harder to discover, i.e. --extend-select FMT is not intuitive
    • However, users are likely to be using ruff format separately when getting started
    • However, if formatting is on by default in the future then opt-out with --ignore FMT is simple
  • Easy for us to add this to the default rule set in the future

The problem with this approach is that there are many special cases where the formatter does not behave like a lint rule. While it's feasible to just push the formatter into the linter, we want to do better.

jaap3 commented 1 year ago

I have no strong opinion, just fyi. I always used https://pypi.org/project/flake8-isort/ and https://pypi.org/project/flake8-black/. Basically the lining check for those plugins is "would isort or black make any changes?". To me that was good enough, never considered adding suppression comment (failure means, just run the formatter).

Sarcasm commented 1 year ago

FWIW, the way I use ruff is wrapped in 2 commands:

The lint command which runs:

ruff check --show-source --show-fixes ...
black --check --diff --quiet ...
# and other commands for other languages than python

I use this in CI for example. I guess I could have used check too but it predates my use of ruff (and I kinda like lint as an intent to verify the code, not act on it, it's a read-only/safe command always).

The fix command which runs:

ruff check --fix-only --select F401,I001 ...
black ...
# and other commands for other languages than python

I use this when I want to cleanup my code, e.g. before commit.

I like that fix is a top level command instead of a check option, because checking and fixing are different intentions IMHO. But I also combines the 2 commands often, where I fix then lint all at once.

I say this because a top-level fix command that do formatting + autofixing of lint issues is maybe less weird than check --fix --format, it's easy to type/remember.

zanieb commented 1 year ago

I also have a long proposal adding a ruff fix command to resolve some of the awkwardness of putting everything in check — it's hard to be certain it will be worth the investment though. I'm glad to hear you think it'd be helpful!

I think regardless there needs to be a way for ruff check to indicate when files are not formatted.

thernstig commented 11 months ago

@zanieb I believe ruff check should be deprecated altogether, and you should use ruff fix to do both ruff format and ruff lint ---fix (https://github.com/astral-sh/ruff/issues/8535). With the caveat that it is clearly documented that since ruff fix would do ruff lint --fix, it could be a potentially dangerous operation if the lint changes are not reviewed.

(Maybe there is a better term then both ruff check and ruff fix we have not thought of?)

jhossbach commented 11 months ago

@zanieb Did you discuss whether this unified command will be implemented in the near future? I am asking because I am considering merging https://github.com/python-lsp/python-lsp-ruff/pull/57 as an intermediary solution.

zanieb commented 11 months ago

@jhossbach I'd recommend moving forward with an intermediary solution as there's still quite a bit of work to be done on this issue.

T-256 commented 10 months ago

FWIW Hatch did it: https://hatch.pypa.io/latest/cli/reference/#hatch-fmt

shayn-orca commented 9 months ago

This would be a good QoL fix for us. Is there an open PR we could jump on this fix this, or is there stuff to be hashed out in the discussion still?

zanieb commented 9 months ago

We need to do some design work around this still and reach consensus internally. We're focusing on stabilizing the formatter first.

Avasam commented 8 months ago

(copying my comment over since the other issue was closed in favor of this one) https://github.com/astral-sh/ruff/issues/8535#issuecomment-1858925442

My thoughts: ruff lint: only lints by default ruff format: formats by default ruff check: shorthand for ruff lint && ruff format --check ruff fix: shorthand for ruff lint --fix && ruff format

shayn-orca commented 8 months ago

@Avasam My only question here is why would I ever want to run ruff lint without ruff format --check? As far as I'm concerned, there are only two states - code it wrong or code is right. Why does the classification between lint and format matters?

MichaReiser commented 8 months ago

@shayn-orca I think that's generally right and check would probably become the most used command. However, I think there are a couple of workflows that are worth considering:

I don't know the right answers to this. Maybe we end up with a single command or we keep separate commands. I think a lot of this depends on how fast ruff is when we add more involved analysis (and plugins?). It might also be easier to start with separate commands as an intermediate step (which also feels more familiar to users) before making the big step to a single command.

shayn-orca commented 8 months ago

Let's imagine that ruff does type checking too and you're in the middle of a larger refactor but you're done refactoring it locally and want to verify if it works as expected. Running the type checker isn't that useful because you know that there are plenty of type checking errors. But let's run the formatter and see if there any lints.

That's true for any subset of linters, not just format VS lint. When you're doing such a large refactor, picking the linters you want to run with a separate config file/CLI args is a common way out as an escape hatch.

Adding more involved analysis like multifile analysis or type checking will inevitably make ruff check slower and you don't want to wait for all the analysis to complete if you only want to format your files

  1. Ruff is so fast, I don't care
  2. Just fail fast by putting the faster lints first in the pipeline?

One challenge with unifying the commands under ruff check is that format writes by default

I mean ruff format --check, which doesn't write anything.

Avasam commented 8 months ago

@Avasam My only question here is why would I ever want to run ruff lint without ruff format --check? [...] ]Why does the classification between lint and format matters?

Because Ruff isn't the only formatter/style checker, and forcing the formatter with the linter only hurts adoption. Even for someone use both, tooling integration may demand to keep those somewhat separate.

mmerickel commented 8 months ago

Because Ruff isn't the only formatter/style checker, and forcing the formatter with the linter only hurts adoption. Even for someone use both, tooling integration may demand to keep those somewhat separate.

That just indicates that the formatter should be opt-in exactly like every other rule in ruff right now. Not that it needs to be a completely separate subcommand.

hmvp commented 3 months ago

Another datapoint here:

I want to configure ruff to run in the gitlab CI. ruff check supports --output-format=gitlab, ruff format does not..
Also I need to run two commands in one step, so even if format would support gitlab output I still need to merge both (or run two job with the extra overhead)

Regardless of the discussion related to the structure of commands, it would help me in this if rust check would have a lint rule that is basically "Run ruff format --check" and report if a file is not formatted... Similar to the isort rule...

AndreuCodina commented 2 weeks ago

In .NET we have a single command for the linter, type checker and formatter with dotnet build (https://learn.microsoft.com/en-us/community/content/how-to-enforce-dotnet-format-using-editorconfig-github-actions#enforcing-code-style-on-build). Meanwhile you can run them separately, it's a cognitive overhead for me.