Skip to content

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

Merged
cjerdonek merged 2 commits into
pypa:masterfrom
cjerdonek:simplify-get-abi-tag-conditionals
Aug 18, 2019
Merged

Simplify get_flag() conditionals in get_abi_tag() [pep425tags]#6889
cjerdonek merged 2 commits into
pypa:masterfrom
cjerdonek:simplify-get-abi-tag-conditionals

Conversation

@cjerdonek

Copy link
Copy Markdown
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
Comment thread src/pip/_internal/pep425tags.py Outdated
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
Copy Markdown
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
Copy Markdown
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. :)

Comment thread src/pip/_internal/pep425tags.py Outdated
@@ -115,18 +115,12 @@ def get_abi_tag():
lambda: hasattr(sys, 'gettotalrefcount'),
warn=(impl == 'cp')):

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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).

@chrahunt chrahunt Aug 17, 2019

Copy link
Copy Markdown
Member

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.

@chrahunt chrahunt left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

2 participants