Skip to content

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

Merged
ntBre merged 5 commits into
mainfrom
brent/pep695-string-fix
Jan 31, 2025
Merged

ntBre merged 5 commits into
mainfrom
brent/pep695-string-fix

Conversation

@ntBre

@ntBre ntBre commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

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.

@github-actions

github-actions Bot commented Jan 30, 2025 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

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
@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)
@MeGaGiGaGon MeGaGiGaGon mentioned this pull request Jun 19, 2025
21 of 71 tasks
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