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

Remove per-diagnostic check for fixability #7919

Merged
merged 4 commits into from
Oct 11, 2023
Merged

Remove per-diagnostic check for fixability #7919

merged 4 commits into from
Oct 11, 2023

Conversation

charliermarsh
Copy link
Member

Summary

Throughout the codebase, we have this pattern:

let mut diagnostic = ...
if checker.patch(Rule::UnusedVariable) {
    // Do the fix.
}
diagnostics.push(diagnostic)

This was helpful when we computed fixes lazily; however, we now compute fixes eagerly, and this is only used to ensure that we don't generate fixes for rules marked as unfixable.

We often forget to add this, and it leads to bugs in enforcing --unfixable.

This PR instead removes all of these checks, moving the responsibility of enforcing --unfixable up to check_path. This is similar to how @zanieb handled the --extend-unsafe logic: we post-process the diagnostics to remove any fixes that should be ignored.

if !settings.rules.should_fix(diagnostic.kind.rule()) {
diagnostic.fix = None;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the actual code change.

Copy link
Member Author

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

This is a five line change, which then requires removing those if guards in nearly every rule.

@charliermarsh charliermarsh added the internal An internal refactor or improvement label Oct 11, 2023
@charliermarsh charliermarsh merged commit c38617f into main Oct 11, 2023
15 checks passed
@charliermarsh charliermarsh deleted the charlie/fix branch October 11, 2023 16:09
@github-actions
Copy link
Contributor

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants