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

Categorise fixes as safe or maybe_incorrect #4184

Closed
Tracked by #4181
MichaReiser opened this issue May 2, 2023 · 9 comments · Fixed by #5348
Closed
Tracked by #4181

Categorise fixes as safe or maybe_incorrect #4184

MichaReiser opened this issue May 2, 2023 · 9 comments · Fixed by #5348
Assignees
Labels
help wanted Contributions especially welcome internal An internal refactor or improvement

Comments

@MichaReiser
Copy link
Member

MichaReiser commented May 2, 2023

Part of #4181 and depends on #4183

#4183 introduced the new Fix::safe and Fix::maybe_incorrect constructors. The goal of this task is to go through all usages of Fix::unspecified, Fix::unspecified_edits, Diagnostic::try_set_from_edit, and Diagnostic::set_from_edit and replace them with Fix::safe* or Fix::maybe_incorrect depending on whether the fix is safe or unsafe (may introduce semantic changes).

I recommend creating individual PRs per rule because it makes discussing the safety properties of individual fixes easier.

@zanieb
Copy link
Member

zanieb commented May 14, 2023

Note the constructor names have changed:

  • Fix::safe -> Fix::automatic
  • Fix::maybe_incorrect -> Fix::suggested or Fix::manual
  • Diagnostic::try_set_from_edit -> Diagnostic::try_set_fix_from_edit

See #4303 and #4275 for details.

@sladyn98
Copy link
Contributor

@MichaReiser I was going through the codebase and it seems all of the code has been migrated, is there something that I am missing with this migration.

@MichaReiser
Copy link
Member Author

@MichaReiser I was going through the codebase and it seems all of the code has been migrated, is there something that I am missing with this migration.

You can search for usages of Fix::unspecified or Fix::unspecified_edits to find rules that need to be migrated

https://github.com/charliermarsh/ruff/blob/c6e5fed6587c95839a814ee8dfa845b465351229/crates/ruff_diagnostics/src/fix.rs#L40-L59

@evanrittenhouse
Copy link
Contributor

evanrittenhouse commented Jun 15, 2023

#5119 depends on all Fix::unspecified instances being refactored to an applicable (pun intended :P ) Applicability. Currently, ripgrep says there are occurrences in:

  1. flake8_commas: fixed in Add Applicability to flake8_comma fixes #5127
  2. flake8_logging_format: fixed in Add Applicability to flake8_logging_format fixes #5129
  3. flake8_pytest_style:
  4. flake8_quotes: fixed in Add Applicability to flake8_quotes fixes #5130
  5. flake8_simpilfy:
  6. flake8_tidy_imports:
  7. flynt
  8. isort
  9. pandas_vet
  10. pycodestyle
  11. pydocstyle
  12. pyflakes
  13. pylint
  14. pyupgrade

An up-to-date list can be found in the description for #5119.

charliermarsh pushed a commit that referenced this issue Jun 15, 2023
charliermarsh pushed a commit that referenced this issue Jun 15, 2023
charliermarsh pushed a commit that referenced this issue Jun 15, 2023
charliermarsh pushed a commit that referenced this issue Jun 15, 2023
charliermarsh pushed a commit that referenced this issue Jun 17, 2023
## Summary

Fixes some of #4184.
charliermarsh pushed a commit that referenced this issue Jun 17, 2023
## Summary

Fixes some of #4184.
charliermarsh pushed a commit that referenced this issue Jun 17, 2023
@evanrittenhouse
Copy link
Contributor

@charliermarsh I believe there are two more linters that need to be completed - pycodestyle and flake8_pytest_style. May want to reopen until those are done, though I plan on completing them by the end of the weekend.

@charliermarsh charliermarsh reopened this Jun 23, 2023
@charliermarsh
Copy link
Member

Oh sorry, did this get closed accidentally by a PR? Anyway, reopened!

@zanieb
Copy link
Member

zanieb commented Jun 28, 2023

I think this is done now! @evanrittenhouse let me know if that's wrong — thanks for taking all those on!

@zanieb zanieb closed this as completed Jun 28, 2023
@evanrittenhouse
Copy link
Contributor

Yup! On to the fun part ;)

@MichaReiser
Copy link
Member Author

Wohoo. Nice work @evanrittenhouse.

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

Successfully merging a pull request may close this issue.

5 participants