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

More compatibility with pep425tags #213

Merged
merged 11 commits into from
Oct 2, 2019
Merged

Conversation

di
Copy link
Member

@di di commented Sep 27, 2019

This PR adds logging around get_config_var as mentioned here: pypa/pip#6908 (comment)

It also fixes #171.

(cc @pradyunsg @cjerdonek)

packaging/tags.py Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
@brettcannon
Copy link
Member

Comments are all stylistic.

I will also say that I initially left the logging out as I wasn't sure how useful it would be since I didn't notice logging being used in 'packaging' itself.

@di
Copy link
Member Author

di commented Sep 30, 2019

I will also say that I initially left the logging out as I wasn't sure how useful it would be since I didn't notice logging being used in 'packaging' itself.

Hmm, maybe we should slightly change the API that pip is expecting here and make the default warn=False? That way anyone else using packaging doesn't get unexpected log messages.

@brettcannon
Copy link
Member

@di yeah, if we are going to bother to include it I think it should be off since it's only helpful when debugging and with 'packaging' as a library I think it should be silent by default.

@di di force-pushed the more-pip-compatibility branch 2 times, most recently from 3116ef0 to 17a0fcd Compare October 1, 2019 21:08
packaging/tags.py Show resolved Hide resolved
@@ -101,6 +105,16 @@ def parse_tag(tag):
return frozenset(tags)


def _get_config_var(name, warn=False):
Copy link
Member

Choose a reason for hiding this comment

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

So with this being false, how will this ever be true such that logging actually occurs? Is the assumption people will manually change locally when they need to? Or is it in case logging comes in the future for the whole the project or through some API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, great point. I think what would be best is that users can turn on logging via an argument to sys_tags and this is propagated through. I feel like this would be easiest to integrate into pip and would have the least effect on other packaging.tags users. I added a commit which does this.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

The code changes LGTM. 2 blocker concerns:

  • docs: we'd want to document the new warn parameter (packaging.tags is pretty well documented so this is in line with that)
  • tests: the tests seem to be failing on the CI for pypy

@pradyunsg pradyunsg merged commit f36ecdf into pypa:master Oct 2, 2019
@pradyunsg
Copy link
Member

Thanks @di! ^>^

@brettcannon I merged on the assumption that you're OK with post-your-approval changes, and if you're not, we can iterate on/discuss reverting specific commits if needed (though I doubt that's the case here). :)

@brettcannon
Copy link
Member

@pradyunsg I'm fine with it. 😄

@di di deleted the more-pip-compatibility branch October 2, 2019 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port changes to reduce dependencies on ctypes module in packaging.tags.
3 participants