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

Refine the warnings about incompatible linter options #8196

Merged
merged 5 commits into from Oct 26, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Oct 25, 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.

@MichaReiser
Copy link
Member Author

MichaReiser commented Oct 25, 2023

Rule::OverIndented,
Rule::IndentWithSpaces,
for rule in [
// The formatter might collapse implicit string concatenation on a single line.
Rule::SingleLineImplicitStringConcatenation,
Copy link
Member Author

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.

Copy link

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.

@MichaReiser MichaReiser force-pushed the refine-format-isort-warning branch 2 times, most recently from 55ecede to 27257e6 Compare October 25, 2023 07:03
Base automatically changed from refine-format-isort-warning to main October 25, 2023 07:41
@MichaReiser MichaReiser force-pushed the refine-linter-incompatible-option-warnings branch 2 times, most recently from 08adb86 to 2d34854 Compare October 25, 2023 08:03
@MichaReiser MichaReiser added cli Related to the command-line interface formatter Related to the formatter and removed cli Related to the command-line interface labels Oct 25, 2023
@MichaReiser MichaReiser marked this pull request as ready for review October 25, 2023 08:08
@github-actions
Copy link

github-actions bot commented Oct 25, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@yakMM
Copy link

yakMM commented Oct 25, 2023

Looks great, most of my warnings are gone!

@MichaReiser
Copy link
Member Author

yakMM wrote

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.

Rule::LineTooLong,
Rule::TabIndentation,
Rule::IndentationWithInvalidMultiple,
Rule::IndentationWithInvalidMultipleComment,
Copy link
Member

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.

@charliermarsh charliermarsh enabled auto-merge (squash) October 26, 2023 16:02
Rule::MissingTrailingComma,
Rule::ProhibitedTrailingComma,
Copy link
Member

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?

Copy link
Member

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.

@charliermarsh charliermarsh merged commit a4dd1e5 into main Oct 26, 2023
16 checks passed
@charliermarsh charliermarsh deleted the refine-linter-incompatible-option-warnings branch October 26, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants