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

Add settings for promoting and demoting fixes #7841

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Oct 6, 2023

Adds two configuration-file only settings extend-safe-fixes and extend-unsafe-fixes which can be used to promote and demote the applicability of fixes for rules.

Fixes with Never applicability cannot be promoted.

.contains(diagnostic.kind.rule())
&& fix.applicability().is_always()
{
diagnostic.set_fix(fix.clone().with_applicability(Applicability::Sometimes))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this clone matter? Is there a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would work:

            if let Some(fix) = diagnostic.fix.take() {
                // Check unsafe before safe so if someone puts a rule in both we are conservative
                if settings
                    .extend_unsafe_fixes
                    .contains(diagnostic.kind.rule())
                    && fix.applicability().is_always()
                {
                    diagnostic.set_fix(fix.with_applicability(Applicability::Sometimes));
                } else if settings.extend_safe_fixes.contains(diagnostic.kind.rule())
                    && fix.applicability().is_sometimes()
                {
                    diagnostic.set_fix(fix.with_applicability(Applicability::Always));
                } else {
                    diagnostic.set_fix(fix);
                }
            }

There's no Fix::set_applicability(&mut self, ...) (only a mut self method), so usual the if let Some(fix) = &mut diagnostic.fix trick doesn't work here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Should I have made set_applicability take &mut self?

Comment on lines +1112 to +1221
let ruff_toml = tempdir.path().join("ruff.toml");
fs::write(
&ruff_toml,
r#"
[lint]
extend-unsafe-fixes = ["UP034"]
"#,
)?;
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 copied this testing strategy from the formatter integration tests. Do we have a better place to do this? I'd test more cases (e.g. prefixes, rules in both sets, etc.)

@zanieb zanieb force-pushed the zanie/app-settings branch 2 times, most recently from e834178 to d7140c4 Compare October 6, 2023 20:08
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@zanieb zanieb marked this pull request as ready for review October 10, 2023 16:12
@zanieb zanieb added the configuration Related to settings and configuration label Oct 10, 2023
crates/ruff_linter/src/linter.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/linter.rs Show resolved Hide resolved
@zanieb zanieb enabled auto-merge (squash) October 10, 2023 19:57
@zanieb zanieb merged commit 739a8aa into main Oct 10, 2023
15 checks passed
@zanieb zanieb deleted the zanie/app-settings branch October 10, 2023 20:04
konstin pushed a commit that referenced this pull request Oct 11, 2023
Adds two configuration-file only settings `extend-safe-fixes` and
`extend-unsafe-fixes` which can be used to promote and demote the
applicability of fixes for rules.

Fixes with `Never` applicability cannot be promoted.
zanieb added a commit that referenced this pull request Oct 11, 2023
Adds documentation for using `ruff check . --fix`

Uses the draft of the "Automatic fixes" section from
#7732 and adds documentation for
unsafe fixes, applicability levels, and
#7841

I enabled admonitions because they're nice. We should use them more.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants