-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refine the warnings about incompatible linter options #8196
Refine the warnings about incompatible linter options #8196
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
3a2dac1
to
142cc30
Compare
Rule::OverIndented, | ||
Rule::IndentWithSpaces, | ||
for rule in [ | ||
// The formatter might collapse implicit string concatenation on a single line. | ||
Rule::SingleLineImplicitStringConcatenation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm undecided if we should warn about this rule. The formatter might collapse implicit string concatenated lines which then triggers this rule. However, it might be something users want to fix manually.
Same for MissingTrailingComma
. The rule might require manual intervention (and magic-trailing-comma
set to respect
), but once fixed it remains fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the formatter collapses the strings, I would love to know about it to fix it afterward.
55ecede
to
27257e6
Compare
08adb86
to
2d34854
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
Looks great, most of my warnings are gone! |
|
2d34854
to
5aaa44d
Compare
Rule::LineTooLong, | ||
Rule::TabIndentation, | ||
Rule::IndentationWithInvalidMultiple, | ||
Rule::IndentationWithInvalidMultipleComment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I these think these are redundant when using tabs in the formatter.
Rule::MissingTrailingComma, | ||
Rule::ProhibitedTrailingComma, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was removed because the formatter will never output code that triggers it? Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe this is the case.
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 itTabIndentation
,IndentWithSpaces
: Only warn ifindent-style="tab"
IndentationWithInvalidMultiple
,IndentationWithInvalidMultipleComment
: Only warn ifindent-width != 4
OverIndented
: Don't warn, but mention that the rule is redundantBadQuotesInlineString
: Warn if quote setting is different fromformat.quote-style
BadQuotesMultilineString
,BadQuotesDocstring
: Warn ifquote != "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.