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

[pyupgrade] Reuse replacement logic from UP046 and UP047 (UP040) #15840

Merged
merged 5 commits into from
Jan 31, 2025

Conversation

ntBre
Copy link
Contributor

@ntBre ntBre commented Jan 30, 2025

Summary

This is a follow-up to #15565, tracked in #15642, to reuse the string replacement logic from the other PEP 695 rules instead of the Generator, which has the benefit of preserving more comments. However, comments in some places are still dropped, so I added a check for this and update the fix safety accordingly. I also added a ## Fix safety section to the docs to reflect this and the existing isinstance caveat.

Test Plan

Existing UP040 tests, plus some new cases.

Copy link
Contributor

github-actions bot commented Jan 30, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@@ -208,9 +227,9 @@ pub(crate) fn non_pep695_type_alias(checker: &mut Checker, stmt: &StmtAnnAssign)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unrelated to my changes here, but I think this TODO is actually done? All of the TODO test cases added in #6314 were fixed in #6749, but maybe there are more types of generics that are still not handled.

// TODO(zanie): We should check for generic type variables used in the value and define them
// as type params instead

Copy link
Member

Choose a reason for hiding this comment

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

Ooh interesting, this might well be done now. I can check in the morning. Doesn't need to hold up this PR though, I don't think!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this TODO seems done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! I'll delete it and then get ready to merge. Thanks again for the review and for checking this!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Fantastic! Simplifies the code, improves the quality of the fix, improves the rule's docs and improves our test coverage 🚀

@AlexWaygood AlexWaygood added the fixes Related to suggested fixes for violations label Jan 30, 2025
@ntBre ntBre merged commit f1418be into main Jan 31, 2025
21 checks passed
@ntBre ntBre deleted the brent/pep695-string-fix branch January 31, 2025 13:10
dcreager added a commit that referenced this pull request Jan 31, 2025
* main:
  [`flake8-pyi`] Fix several correctness issues with `custom-type-var-return-type` (`PYI019`) (#15851)
  [`pyupgrade`] Reuse replacement logic from `UP046` and `UP047` (`UP040`) (#15840)
  [`refurb`] Avoid `None | None` as well as better detection and fix (`FURB168`) (#15779)
  Remove non-existing `lint.extendIgnore` editor setting (#15844)
  [`refurb`] Mark fix as unsafe if there are comments (`FURB171`) (#15832)
  [`flake8-comprehensions`] Skip when `TypeError` present from too many (kw)args for `C410`,`C411`, and `C418` (#15838)
  [`pyflakes`] Visit forward annotations in `TypeAliasType` as types (`F401`) (#15829)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants