Skip to content

Conversation

@BruceForstall
Copy link
Contributor

Updated a few comments as well.

@BruceForstall BruceForstall requested a review from jashook December 5, 2019 01:59
""" Main method
"""

# await/async requires python >= 3.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind changing the comment to >= 3.8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to force 3.8+? I made the comment say 3.8, but I'm not sure it actually requires anything more than 3.5 (you said it needs that for async)

Copy link
Contributor

Choose a reason for hiding this comment

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

It technically requires 3.7 as 3.6 on windows has an async bug.

"""

# await/async requires python >= 3.5
if sys.version_info.major < 3 and sys.version_info.minor < 5:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also could you change the check to sys.version_info.minor < 8

Copy link
Contributor

@jashook jashook left a comment

Choose a reason for hiding this comment

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

Other than that lgtm thank you!

@BruceForstall BruceForstall merged commit 8f67999 into dotnet:master Dec 5, 2019
@BruceForstall BruceForstall deleted the FixSuperpmiPyForNewRepo branch December 5, 2019 18:14
@BruceForstall
Copy link
Contributor Author

I went ahead and merged. If you want to force 3.8+ instead of 3.5+ as it is now, I'll do that in a separate change.

@jashook
Copy link
Contributor

jashook commented Dec 5, 2019

We should force 3.7

@sandreenko sandreenko mentioned this pull request Dec 7, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
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.

2 participants