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.

Sign up for GitHub

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

Jump to bottom

Use variable annotations #10723

Merged
merged 10 commits into from
Jun 28, 2021
Merged

Use variable annotations #10723

merged 10 commits into from
Jun 28, 2021

Conversation

hauntsaninja
Copy link
Collaborator

No description provided.

@TH3CHARLie
Copy link
Collaborator

Glad to see we are moving in this direction. It's better if this can be split into several PRs. Did you use scripts to automate this?

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Jun 28, 2021 •
edited
Loading

Yes, you can see the exact history of what I did in the commit history, the initial heavy lifting is done by com2ann. I'm not sure that breaking this up into multiple PRs would really help. If you want to go over this with a fine toothed comb, the Github "viewed" functionality for each file can be useful. But overall, since this is just changing annotations and not really code (I only removed default Nones if they were incompatible with the type), and type checking and tests pass, I feel pretty confident in this PR.

@TH3CHARLie
Copy link
Collaborator

Agreed. I'll start to review this and I am also confident this is a No functional change (NFC)

TH3CHARLie
TH3CHARLie approved these changes Jun 28, 2021
View reviewed changes
Copy link
Collaborator

@TH3CHARLie TH3CHARLie left a comment

Choose a reason for hiding this comment

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

Looks good to me! I've noticed several manual fixes, thanks for taking the time to do this!

@TH3CHARLie
Copy link
Collaborator

TH3CHARLie commented Jun 28, 2021 •
edited
Loading

I believe changeset at this scale requires at least one more approval.

ilevkivskyi
ilevkivskyi approved these changes Jun 28, 2021
View reviewed changes
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Nice!

@TH3CHARLie TH3CHARLie merged commit f98f782 into python:master Jun 28, 2021
@hauntsaninja hauntsaninja deleted the com2ann branch June 28, 2021 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TH3CHARLie TH3CHARLie TH3CHARLie approved these changes

@ilevkivskyi ilevkivskyi ilevkivskyi approved these changes

@JukkaL JukkaL Awaiting requested review from JukkaL

@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hauntsaninja @TH3CHARLie @ilevkivskyi