-
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
Avoid duplicating linter-formatter compatibility warnings #8292
Conversation
8c06ba2
to
8b00e92
Compare
Rule::IndentationWithInvalidMultipleComment, | ||
]) && setting.formatter.indent_width.value() != 4 | ||
{ | ||
warn_user_once!("The `format.indent-width` option with a value other than 4 is incompatible with `E111` and `E114`. We recommend disabling these rules when using the formatter, which enforces a consistent indentation width. Alternatively, set the `format.indent-width` option to `4`."); | ||
} |
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.
Gave this a custom warning based on the indent-width.
if setting.linter.rules.enabled(Rule::TabIndentation) | ||
&& setting.formatter.indent_style.is_tab() | ||
{ | ||
warn_user_once!("The `format.indent-style=\"tab\"` option is incompatible with `W191`. We recommend disabling these rules when using the formatter, which enforces a consistent indentation style. Alternatively, set the `format.indent-style` option to `\"space\"`."); |
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.
Gave this a custom warning based on the indent-style.
8b00e92
to
c3e9d39
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no linter changes. |
.map(|rule| format!("`{}`", rule.noqa_code())) | ||
.collect(); | ||
rule_names.sort(); | ||
warn_user_once!("The following rules may cause conflicts when used with the formatter: {}. 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.", rule_names.join(", ")); |
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.
Minor question — since warn_user_once
works using the call site, does this mean if we find a second set of incompatible rules in a different pyproject file we will not emit a second message with the different set? Probably not common... but not ideal?
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.
No, for this specific warning, we iterate over all pyproject.toml
and collect all incompatible rules, so they would all be flagged here.
(All the other warnings are static, this is the only warning with dynamic input.)
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.
Sounds good!
I'd like to double check the removal of the on Monday. I verified them all manually and believe that I was able to come up with a conflict |
@MichaReiser - Sorry, you're right. I think we don't reformat module-level docstrings which is why I was seeing different results. If we re-indent a function or class docstring, we do indeed change from spaces to tabs. Will restore. |
c3e9d39
to
a7cc846
Compare
@MichaReiser - Restored. |
1c72bbe
to
2d903b9
Compare
Summary
Uses
warn_user_once!
instead ofwarn!
to ensure that every warning is shown exactly once, regardless of whether there are duplicates in the list, or warnings that are raised by multiple configuration files.Closes #8271.