-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add support for collections.abc.Collection type hints #29272
Conversation
Codecov Report
@@ Coverage Diff @@
## master #29272 +/- ##
=======================================
Coverage 38.30% 38.30%
=======================================
Files 690 690
Lines 102043 102072 +29
=======================================
+ Hits 39085 39097 +12
- Misses 61375 61392 +17
Partials 1583 1583
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
Assigning reviewers. If you would like to opt out of this review, comment R: @johnjcasey added as fallback since no labels match configuration Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, had a couple minor questions/comments
is_consistent_with(elem, self.inner_type) | ||
for elem in sub.tuple_types) | ||
elif not isinstance(sub, TypeConstraint): | ||
if getattr(sub, '__origin__', None) is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this check? I would've assumed issubclass
would take care of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameterized generics and issubclass
do not get along. If you pass along a parameterized type (something like issubclass(set[int], collections.abc.Collection)
) you actually get an error that the first arg has to be a class. This is why the internal checking for these containers separates out the container type from the type it wraps and checks them separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it - thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds a new internal Beam representation for type hints that are exactly
collections.abc.Collections
types. Allows for more general type hinting guarantees.Part of #29135
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.