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

Revert Pyupgrade Pep 604 Typing Rewrites #562

Merged
merged 9 commits into from
Jan 12, 2024
Merged

Conversation

collindutter
Copy link
Member

Reverts pyupgrade's pep 604 typing rewrites which causes issues with #561 on versions greater than python 3.9.

Thank goodness for regexes.

@collindutter collindutter requested a review from a team January 11, 2024 23:55
@collindutter collindutter marked this pull request as draft January 12, 2024 00:07
@collindutter collindutter marked this pull request as ready for review January 12, 2024 00:56
@collindutter
Copy link
Member Author

So this is really only an issue in the context of #561 when the union syntax is used in a field since those are the types that are resolved when building a schema.

Feels kind of weird to require Union for fields, but allow | syntax elsewhere. Should we just drop 3.9?

@collindutter
Copy link
Member Author

collindutter commented Jan 12, 2024

Copy link
Member

@SavagePencil SavagePencil left a comment

Choose a reason for hiding this comment

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

Two questions that do not impact approval but want addressed before we move forward with this one.

tests/unit/mixins/test_seriliazable_mixin.py Outdated Show resolved Hide resolved
griptape/artifacts/blob_artifact.py Show resolved Hide resolved
SavagePencil
SavagePencil previously approved these changes Jan 12, 2024
Copy link
Member

@SavagePencil SavagePencil left a comment

Choose a reason for hiding this comment

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

bless this mess

@andrewfrench
Copy link
Member

andrewfrench commented Jan 12, 2024

So this is really only an issue in the context of #561 when the union syntax is used in a field since those are the types that are resolved when building a schema.

Feels kind of weird to require Union for fields, but allow | syntax elsewhere. Should we just drop 3.9?

That does feel weird, and maybe would be easier to drop 3.9. It'd be an easier decision if we had any idea what the version breakdown was among users. I found these survey results from Jetbrains (the 2023 survey results aren't available yet) implying 3.9 has a pretty large share, but clearly pretty out of date by now as it only includes versions as high as 3.10.

@collindutter
Copy link
Member Author

Another option could be to go all in on the revert so consistent throughout. I only reverted to T | None to Optional[T]. We could also revert T | U | V to Union[T, U, V], but this would undo a decision made in #226.

Dropping 3.9 feels wrong, also I think 3.10 still has some issues with | syntax.

andrewfrench
andrewfrench previously approved these changes Jan 12, 2024
Copy link
Member

@andrewfrench andrewfrench left a comment

Choose a reason for hiding this comment

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

Nothing worth blocking approval over but several unused Optional imports added here.

The question around 3.9 support generally is worth discussing but can happen outside this PR.

@collindutter collindutter merged commit f2e10e3 into dev Jan 12, 2024
5 of 6 checks passed
@collindutter collindutter deleted the fix/revert-types branch January 12, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants