Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unified command for linting and formatting #8232

Open
zanieb opened this issue Oct 25, 2023 · 22 comments
Open

Unified command for linting and formatting #8232

zanieb opened this issue Oct 25, 2023 · 22 comments
Labels
cli Related to the command-line interface formatter Related to the formatter

Comments

@zanieb
Copy link
Member

zanieb commented Oct 25, 2023

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 #7310 (reply in thread), #7310 (comment), and #7232

@zanieb zanieb added this to the Formatter: Stable milestone Oct 25, 2023
@zanieb
Copy link
Member Author

zanieb commented Oct 25, 2023

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
Copy link
Contributor

T-256 commented Oct 26, 2023

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 #7310 (reply in thread), 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
Copy link
Member

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
  • The CLI would be fundamentally different from how editors work where editors use different commands for formatting and linting.
  • How would we support enabling preview style formatting for the formatter without enabling preview behavior of other lint rules.

Overall, formatting just felt different enough.

@hmvp
Copy link

hmvp commented Oct 26, 2023

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
Copy link
Contributor

T-256 commented Oct 26, 2023

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
Copy link
Member Author

zanieb commented Oct 26, 2023

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
Copy link
Contributor

jaap3 commented Oct 27, 2023

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
Copy link

Sarcasm commented Oct 27, 2023

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
Copy link
Member Author

zanieb commented Oct 27, 2023

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
Copy link

thernstig commented Nov 8, 2023

@zanieb I believe ruff check should be deprecated altogether, and you should use ruff fix to do both ruff format and ruff lint ---fix (#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
Copy link

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

@zanieb
Copy link
Member Author

zanieb commented Nov 15, 2023

@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
Copy link
Contributor

T-256 commented Dec 14, 2023

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

@shayn-orca
Copy link

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?

@Avasam
Copy link

Avasam commented Feb 23, 2024

(copying my comment over since the other issue was closed in favor of this one) #8535 (comment)

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
Copy link

@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
Copy link
Member

MichaReiser commented Feb 25, 2024

@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:

  • 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.
  • 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
  • The opposite is also true. I almost never run cargo fmt because my IDE keeps the file formatted.
  • One challenge with unifying the commands under ruff check is that format writes by default, whereas lint only checks. I don't know what the best default for incorrectly formatted code should be: Should we show the full diff or only mention that it isn't correctly formatted? The full diff is useful in CI but is probably very noisy when running in the CLI where I'm preliminary interested in lint errors because I trust the formatter. That's where a dedicated lint command could be useful

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
Copy link

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
Copy link

Avasam commented Feb 25, 2024

@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
Copy link
Contributor

mmerickel commented Feb 25, 2024

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
Copy link

hmvp commented Jul 4, 2024

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface formatter Related to the formatter
Projects
None yet
Development

No branches or pull requests