Modernize type hints #31755
Modernize type hints #31755
Conversation
|
R: @tvalentyn |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
robertwb
force-pushed
the
type-hints
branch
3 times, most recently
from
580b599
to
a827861
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31755 +/- ##
=============================================
+ Coverage 71.18% 81.21% +10.02%
=============================================
Files 1058 397 -661
Lines 134254 67717 -66537
Branches 3257 0 -3257
=============================================
- Hits 95571 54997 -40574
+ Misses 35536 12720 -22816
+ Partials 3147 0 -3147
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Run Python 3.8 PostCommit |
|
postcommits will be here: https://github.com/apache/beam/actions/runs/9848030460 there were linter failures as well - have you also tried importing this and running internal tests? |
|
Good call. Internal tests look good on import. |
|
Fixed the lint issue. |
There was a problem hiding this comment.
I looked at manual commits and spot-checked the autogenerated content, but there is a lot of changes and I could have missed something. What is the worst thing that can go wrong here? Inferring incorrect coder because there is a typo in a typehint? Do we need to be more careful and split it up into smaller PRs ?
| @@ -309,8 +310,8 @@ def is_alive(): | |||
| return not (shutdown_requested or evaluation_context.shutdown_requested) | |||
|
|
|||
| # The shared queue that allows the producer and consumer to communicate. | |||
| channel: Queue[Union[test_stream.Event, | |||
| _EndOfStream]] = Queue() # noqa: F821 | |||
| channel: 'Queue[Union[test_stream.Event, _EndOfStream]]' = ( # noqa: F821 | |||
There was a problem hiding this comment.
does the entire hint need to be quoted or only '_EndOfStream' ?
There was a problem hiding this comment.
Also test_stream.Event. Given the noqa, it seems there were issues with this before.
|
Yes, looking at the manual changes and spot checking the auto generated should be sufficient. The worst thing is failure to import because of some circular/forward definition. I think I fixed all of those (even if things aren't imported by default, the docbuilder import most modules.) As you mention, some coders are inferred from type hints, but that should fail quickly because of actually being incorrect (and I didn't run into any of this in all the tests). |
|
Also, if there are issues we should be able to revert individual commits rather than roll the whole thing back. I'll wait 'till the release is cut before merging this to give it extra time to bake. |
Convert from Python2 style comment type hints to true Python 3 annotations.
This was done with com2ann.
This PR only covers those files that have not been edited recently, and is split by top-level directory to reduce the chance and cost of conflicts.
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.mdwith 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.