-
Notifications
You must be signed in to change notification settings - Fork 239
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
fix: bin/update_pythons.py
to work properly with PyPy win_amd64
#695
Conversation
bin/update_pythons.py
Outdated
if identifier.startswith("pp"): | ||
config.update(**self.windows_pypy.update_version_windows(spec)) | ||
else: | ||
config_update = self.windows_64.update_version_windows(spec) | ||
if config_update: | ||
config.update(**config_update) |
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.
if identifier.startswith("pp"): | |
config.update(**self.windows_pypy.update_version_windows(spec)) | |
else: | |
config_update = self.windows_64.update_version_windows(spec) | |
if config_update: | |
config.update(**config_update) | |
config_update: ConfigWinCP | ConfigWinPP | None | |
if identifier.startswith("pp"): | |
config_update = self.windows_pypy.update_version_windows(spec) | |
else: | |
config_update = self.windows_64.update_version_windows(spec) | |
if config_update: | |
config.update(**config_update) |
Not sure this is any better, though.
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.
if identifier.startswith("pp"): | |
config.update(**self.windows_pypy.update_version_windows(spec)) | |
else: | |
config_update = self.windows_64.update_version_windows(spec) | |
if config_update: | |
config.update(**config_update) | |
config_update = ( | |
self.windows_pypy.update_version_windows(spec) | |
if identifier.startswith("pp") | |
else self.windows_64.update_version_windows(spec) | |
) | |
if config_update: | |
config.update(**config_update) |
Assuming this passes MyPy, maybe it's better?
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.
Technically, the current form looks just like the win32 version above, so it's also fine (and if changed, the one above should follow too).
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.
I reworked this a bit. Just fail if config_update is None
as was done on macOS, it shall never happen.
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.
Okay, sounds good.
Follow up to #671, for the record. |
No description provided.