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.

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

Simplify get_flag() conditionals in get_abi_tag() [pep425tags] #6889

Merged
merged 2 commits into from
Aug 18, 2019

Conversation

cjerdonek
Copy link
Member

This simplifies a couple conditionals in pep425tags.py's get_abi_tag() function, as mentioned in this comment to PR #6874.

@cjerdonek cjerdonek added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Aug 17, 2019
@cjerdonek cjerdonek changed the title Simplify the get_flag() conditionals in pep425tags.py's get_abi_tag() Simplify get_flag() conditionals in get_abi_tag() [pep425tags] Aug 17, 2019
sys.version_info < (3, 8))) \
and sys.version_info < (3, 8):
if sys.version_info < (3, 8) and get_flag(
'WITH_PYMALLOC', lambda: impl == 'cp', warn=(impl == 'cp')):
Copy link
Member

Choose a reason for hiding this comment

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

I would do is_cpython = impl == 'cp' above and use it everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was planning on doing exactly this. :)

@@ -115,18 +115,12 @@ def get_abi_tag():
lambda: hasattr(sys, 'gettotalrefcount'),
warn=(impl == 'cp')):
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we only warn for CPython, are these totally sure things on PyPy?

Copy link
Member Author

Choose a reason for hiding this comment

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

For comparison, you may want to look at the newer packaging.tags to see if behavior like this got preserved.

Copy link
Member Author

Choose a reason for hiding this comment

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

The warning is if they’re missing, so I’m guessing it’s because they aren’t known to be required on other implementations (so the warnings would risk being spurious).

Copy link
Member

@chrahunt chrahunt Aug 17, 2019

Choose a reason for hiding this comment

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

In packaging.tags the behavior did not get preserved, which may be a bug on Windows (filed at pypa/packaging#181).


That seems reasonable.

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

LGTM!

@cjerdonek cjerdonek merged commit 0b23e2d into pypa:master Aug 18, 2019
@cjerdonek cjerdonek deleted the simplify-get-abi-tag-conditionals branch August 18, 2019 00:49
@cjerdonek
Copy link
Member Author

Thanks!

pradyunsg pushed a commit to pradyunsg/pip that referenced this pull request Aug 25, 2019
…itionals

Simplify get_flag() conditionals in get_abi_tag() [pep425tags]
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Sep 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants