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

Consider adding an option to supress ruff format CLI warnings #8185

Closed
yakMM opened this issue Oct 24, 2023 · 25 comments
Closed

Consider adding an option to supress ruff format CLI warnings #8185

yakMM opened this issue Oct 24, 2023 · 25 comments
Labels
formatter Related to the formatter needs-decision Awaiting a decision from a maintainer

Comments

@yakMM
Copy link

yakMM commented Oct 24, 2023

Just tried ruff format, it's seriously great!

I'm just annoyed by the warnings showing up in console: warning: The following rules may cause conflicts when used with the formatter: XXX

I want my linter (specifically the isort part) taking priority over the formatter, so I run the linter after the formatter and never had problems.

Is there any way to acknowledge / supress the CLI warnings with a configuration option or a command line parameter?

@MichaReiser MichaReiser added the formatter Related to the formatter label Oct 24, 2023
@MichaReiser MichaReiser modified the milestone: Formatter: Stable Oct 24, 2023
@MichaReiser MichaReiser added the needs-decision Awaiting a decision from a maintainer label Oct 24, 2023
@MichaReiser
Copy link
Member

Thanks for reporting. Which of the conflicting rules do you want to use alongside the formatter and what's the use case for having both enabled (what do they catch in addition to the formatter)?

@charliermarsh
Copy link
Member

(It sounds like one example is: using isort with different settings; so you run the formatter, then linter, to get formatted code with different import settings (which would then be reformatted if you re-ran the formatter).)

@yakMM
Copy link
Author

yakMM commented Oct 24, 2023

Yeah I use force-single-line = true from isort (was using it with black as well).

I also have a warning about a bunch of other conflicts ('D206', 'E501', 'ISC001', 'Q000', 'Q001', 'Q002', 'Q003', 'W191').

My configuration might be weird, I have select = ["ALL"] and a few groups in ignore. I prefer to do it this way because:

  • I get to test all the new rules when updating
  • It's a smaller config to disable the few rules I don't want.

And actually if I stop running ruff check, ruff format is not reformatting anything, so I'm not too sure if there is really a conflict?

Also I would not want to disable all ruff warnings, only these rule conflict warnings.

@acdha
Copy link

acdha commented Oct 24, 2023

I hit this as well, with a very vanilla configuration - no custom isort options, etc. but we have a blanket select = ["E", "W", …] which means I see this:

warning: The following rules may cause conflicts when used with the formatter: 'E501', 'W191'. To avoid unexpected behavior, we recommend disabling these rules, either by removing them from the select or extend-select configuration, or adding then to the ignore configuration.

The W191 seems like it could only be displayed when you have customized the formatter to use tabs instead of spaces.

E501 seems like a harder case since most of the time it won't be an issue but there are a difficult cases (long comments, etc.) where it might be an issue. I wonder whether this might be as simple as implementing #8175 or trying something clever see whether the format + lint combination is stable before emitting it so the warning could be strengthened from “may cause conflicts” to “is causing conflicts with your configuration”.

@MichaReiser
Copy link
Member

Thanks for explaining.

We intentionally allow E501 because some projects use E501 to warn about extra long lines (e.g. line-length: 80 and pycodestyle.max-line-length: 100).

The W191 seems like it could only be displayed when you have customized the formatter to use tabs instead of spaces.

I like this

@MichaReiser
Copy link
Member

#8196 refines the rule warnings and #8192 refined the isort warnings. I hope that this will remove all warnings for you (at least all warnings that you can't suppress)

@yakMM
Copy link
Author

yakMM commented Oct 25, 2023

!!! You guys are fast, well done! It's a bit better, I get different ones now.

I'm still not convinced by the warnings. If we take the linter as an example, there is not such warning and it's up to the user to make sure they didn't activate conflicting rules isn't it?

Moreover, I still don't understand how the rules included in the warning conflict with the formatter with default settings: ('D206', 'E501', 'ISC001', 'Q000', 'Q001', 'Q002', 'Q003', 'W191')

@MichaReiser
Copy link
Member

!!! You guys are fast, well done! It's a bit better, I get different ones now.

Did you build from source? The changes are not yet released.

@yakMM
Copy link
Author

yakMM commented Oct 25, 2023

!!! You guys are fast, well done! It's a bit better, I get different ones now.

Did you build from source? The changes are not yet released.

Yes yes I used the master branch from a few minutes ago. I no longer have the conflict about force-single-line from isort.

@MichaReiser
Copy link
Member

Oh nice! Can you try #8196? It isn't yet merged. Feel free to comment directly on the PR.

@yakMM
Copy link
Author

yakMM commented Oct 25, 2023

Tested on my project, only have a warning about ISC001 and isort.split-on-trailing-comma left. I will reply to your comment about this one directly in the PR.

Don't know if I should open a new issue, but I feel like the warning about isort.split-on-trailing-comma=false is underserved since I already have isort.force-single-line=true, so that isort.split-on-trailing-comma=false will never be applied. I don't know how complicated it is to keep making exceptions to the warning on the implementation side.

@MichaReiser
Copy link
Member

Don't know if I should open a new issue, but I feel like the warning about isort.split-on-trailing-comma=false is underserved since I already have isort.force-single-line=true, so that isort.split-on-trailing-comma=false will never be applied. I don't know how complicated it is to keep making exceptions to the warning on the implementation side.

Could you share your configuration and what warning your seeing. It should never warn about isort.split-on-trailing-comma when set to false

@yakMM
Copy link
Author

yakMM commented Oct 25, 2023

Ah no I don't have isort.split-on-trailing-comma. It's asking me to set it to false.

My config:

[tool.ruff]
line-length = 88

# We select all rules then ignore the one we don't want
select = ["ALL"]
ignore = [
    "COM",      # Commas
    "EM",       # Err-msg
    "INP",      # No-pep420
    "TCH",      # Type-checking, don't want to nest imports under TYPE_CHECKING
    "SLF",      # Self
    "TD",       # Todos
    "FIX",      # Fix-me
    "S101",     # assert, used for type-checking
    "ANN401",   # dynamic typing, already checked by mypy
    "ANN101",   # missing-type-self
    "ANN102",   # missing-type-cls
    "D1",       # Missing docstring
    "TRY003",   # Long messages in exception class
]
target-version = "py312"

[tool.ruff.isort]
force-single-line = true

[tool.ruff.format]
skip-magic-trailing-comma = true

[tool.ruff.flake8-tidy-imports]
ban-relative-imports = "parents"

[tool.ruff.pydocstyle]
convention = "pep257"

@MichaReiser
Copy link
Member

I think that's actually correct.

If you add the trailing comma after the last alias as in this example:

from a import (b, c,)

Then isort formats it like this:

from a import (
    b,
    c,
)

Now, skip-magic-trailing-comma=false is incompatible split-trailing-comma=true because the formatter now undoes the multline formatting, ignoring your intention of adding the trailing comma:

from a import b, c

That's why you either have to override the default of split-trailing-comma and set it to false OR set format.skip-magic-trailing-comma to true.

Either way, I think we should extend the skip-magic-trailing-comma documentation and mention that you have to change split-trailing-comma when setting it to true.

@yakMM
Copy link
Author

yakMM commented Oct 25, 2023

Then isort formats it like this:

from a import (
    b,
    c,
)

But this will not happen because I have force-single-line=True

Instead, isort will format to:

from a import b
from a import c

Which is not a problem for the formatter. Now the question is more "how far should you go in the combination of options"

@charliermarsh
Copy link
Member

Perhaps we could combine this request with a mechanism by which we automatically disable conflicting lint rules and options -- something like formatter-compatibility = warn | ignore | force.

@MichaReiser
Copy link
Member

MichaReiser commented Oct 25, 2023

Perhaps we could combine this request with a mechanism by which we automatically disable conflicting lint rules and options -- something like formatter-compatibility = warn | ignore | force.

That sounds interesting. I recommend making such option tooling independent (e.g. disabling conflicting rules etc). Having a tool specific name also opens the question of where it should be placed in the configuration. Under format, or better lint?

@zanieb
Copy link
Member

zanieb commented Oct 26, 2023

If we take the linter as an example, there is not such warning and it's up to the user to make sure they didn't activate conflicting rules isn't it?

We actually do have warnings in the linter like this too e.g.

❯ ruff check . --select ALL
warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.

I recommend making such option tooling independent

What do you mean? Like ruff.on-tool-configuration-conflict instead of ruff.tool.format.on-linter-conflict?

@MichaReiser
Copy link
Member

MichaReiser commented Oct 26, 2023

I recommend making such option tooling independent

What do you mean? Like ruff.on-tool-configuration-conflict instead of ruff.tool.format.on-linter-conflict?

Kind of, but ideally more generic: configuration-conflict = "warn" | "resolve" Because there are other conflicts. E.g. you could set pycodestyle.line-length to a lower limit than the line-length and that will lead to incompatibility with isort and the formatter. We could probably come up with a better name. There are also conflicts between the same tool as your example shows (conflict between lint rules)

Alternatives: configuration-conflict-resolution = "manual" | "automatic" | "silent" (automatic resolves conflicts but warns). Although I'm not sure how I feel about silent

@zanieb
Copy link
Member

zanieb commented Oct 26, 2023

Hm that seems really broad... I probably want different behavior depending on the category of conflict but perhaps this is a good start. Maybe on-configuration-conflict = "error" | "warn" | "ignore"?

@yakMM
Copy link
Author

yakMM commented Oct 26, 2023

Perhaps we could combine this request with a mechanism by which we automatically disable conflicting lint rules and options -- something like formatter-compatibility = warn | ignore | force.

Well I opened this issue with this in mind, but when I see all the great work aiming at making the warnings represent actual conflicts and remove false-positive, I'm not sure it's really necessary anymore.

Don't know about others, but #8196 and #8244 will probably remove all the warnings for my use cases.

charliermarsh pushed a commit that referenced this issue Oct 26, 2023
## Summary

Avoid warning about incompatible rules except if their configuration
directly conflicts with the formatter. This should reduce the noise and
potentially the need for #8175
and #8185

I also extended the rule and option documentation to mention any
potential formatter incompatibilities or whether they're redundant when
using the formatter.

* `LineTooLong`: This is a use case we explicitly want to support. Don't
warn about it
* `TabIndentation`, `IndentWithSpaces`: Only warn if
`indent-style="tab"`
* `IndentationWithInvalidMultiple`,
`IndentationWithInvalidMultipleComment`: Only warn if `indent-width !=
4`
* `OverIndented`: Don't warn, but mention that the rule is redundant
* `BadQuotesInlineString`: Warn if quote setting is different from
`format.quote-style`
* `BadQuotesMultilineString`, `BadQuotesDocstring`: Warn if `quote !=
"double"`

## Test Plan

I added a new integration test for the default configuration with `ALL`.
`ruff format` now only shows two incompatible rules, which feels more
reasonable.
charliermarsh pushed a commit that referenced this issue Oct 26, 2023
…is true (#8244)

## Summary

Based on [this
feedback](#8185 (comment)).
Avoid warning about `force-wrap-aliases` and `split-on-trailing-comma`
if `force-single-line` is true (which creates a dedicated import for
each imported member).

## Test Plan

Ran `ruff format . --no-cache` and verified that the warning show up
when `force-single-line=false` and aren't shown when
`force-single-line=true`
@MichaReiser
Copy link
Member

We shipped some improvements in the last release. Please open a new issue if you still encounter issues and would prefer having an option to suppress all format CLI warnings.

@yakMM
Copy link
Author

yakMM commented Oct 27, 2023

@MichaReiser Thanks for the work this is great. I see you choose not to add ISC001 in your PR in the end, is there a specific reason to that?

@bswck
Copy link
Contributor

bswck commented Dec 28, 2023

In ruff check, there's always an option to run ruff check . --select=ALL 2>/dev/null if you can't fix the incompatibilities and want to ignore all warnings, as they get written to stderr.

I usually do not use the ruff formatter directly, thus only talking about ruff check.

This is a very "nuclear" practice, but suppresses the annoying warnings.

@yasirroni
Copy link

yasirroni commented Mar 16, 2024

I hit this as well, with a very vanilla configuration - no custom isort options, etc. but we have a blanket select = ["E", "W", …] which means I see this:

warning: The following rules may cause conflicts when used with the formatter: 'E501', 'W191'. To avoid unexpected behavior, we recommend disabling these rules, either by removing them from the select or extend-select configuration, or adding then to the ignore configuration.

The W191 seems like it could only be displayed when you have customized the formatter to use tabs instead of spaces.

E501 seems like a harder case since most of the time it won't be an issue but there are a difficult cases (long comments, etc.) where it might be an issue. I wonder whether this might be as simple as implementing #8175 or trying something clever see whether the format + lint combination is stable before emitting it so the warning could be strengthened from “may cause conflicts” to “is causing conflicts with your configuration”.

What is the recommended solution for this @acdha?


Edit: turns out, simply update to the latest ruff version solved it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter needs-decision Awaiting a decision from a maintainer
Projects
None yet
Development

No branches or pull requests

7 participants