Skip to content

chore: enable pep 563/585 - #5088

Merged
neersighted merged 1 commit into
python-poetry:masterfrom
branchv:pep-563-585
Feb 28, 2022
Merged

neersighted merged 1 commit into
python-poetry:masterfrom
branchv:pep-563-585

Conversation

@branchv

@branchv branchv commented Jan 22, 2022

Copy link
Copy Markdown
Member

This enables PEP 563 and PEP 585 🔥

Blocked by #5055
Relates-To: #4997

@branchv
branchv marked this pull request as draft January 22, 2022 20:28
@branchv
branchv marked this pull request as ready for review January 23, 2022 15:34
@neersighted

Copy link
Copy Markdown
Member

I think this might be a bit too aggressive -- is the new union syntax really available? I see all the tests pass, but according to PEP 604 this wasn't added til 3.10. My memory is a bit rusty however.

@branchv

branchv commented Jan 25, 2022

Copy link
Copy Markdown
Member Author

Yea, you're right technically x: int | str is a syntax error for <3.10. However, with from __future__ import annotations this becomes equivalent to x: "int | str". So by default it's not evaluated at run-time, but mypy knows how to interpret it.

I should have noted that pyupgrade handled this migration, the only other change here was removing unused imports (for which I used pycln).

@neersighted

Copy link
Copy Markdown
Member

A second PR adding pycln to the pre-commit config would be welcome, I think.

Consensus seems to be that the new Union syntax is harmless and nicer to write. Let's go ahead with this.

neersighted
neersighted previously approved these changes Jan 25, 2022

@neersighted neersighted 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.

Approved, but I'd like to wait for at least one other maintainer to eyeball this before we merge.

@danieleades

Copy link
Copy Markdown
Contributor

I don't hate this. I've got a feeling that the pyupgrade hook can also add the from __future__ import annotations line, but perhaps only on later targets (maybe 3.9?). Not sure if anyone can confirm or deny that.

@branchv

branchv commented Jan 25, 2022

Copy link
Copy Markdown
Member Author

pyupgrade hook can also add the from __future__ import annotations line

I checked, it can definitely remove them, but I can't find any indication of it adding them. See asottile/pyupgrade#255 (comment) for an explanation of why it doesn't support that

@branchv

branchv commented Feb 3, 2022

Copy link
Copy Markdown
Member Author

rebased due to conflicts

@branchv
branchv requested a review from neersighted February 3, 2022 22:53
@neersighted
neersighted merged commit da9680b into python-poetry:master Feb 28, 2022
@branchv
branchv deleted the pep-563-585 branch February 28, 2022 05:15
@github-actions

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants