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

Use pip-api instead of pip's internal APIs #59

Merged
merged 3 commits into from
Mar 21, 2018
Merged

Conversation

di
Copy link
Contributor

@di di commented Mar 20, 2018

Fixes #58.

Instead of using pip's internal APIs, which will be moving in pip==10.0.0, use pip-api instead.

Also switches to using the packaging project directly, rather than pip vendored version of it (I didn't see why this would be necessary).

@di
Copy link
Contributor Author

di commented Mar 21, 2018

@peterbe Since this PR would definitely not work for Python 2.6 and 3.3 users, I added a python_requires to hashin's setup.py so that any future distributions wouldn't be installable for those users.

Copy link
Owner

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

I love it!

However, the only reason we now need to depend on another new package is just to be able to throw an error for people using pip 7 and older.

Basically, since we're no longer depending on pip directly (instead we're depending on pip-api) perhaps we don't need to say you have to have pip >=8.

I.e. if you have pip 6 and install hashin and pip-api you'd be run pip_api.hash(filename, algorithm) anyway, right?

@peterbe
Copy link
Owner

peterbe commented Mar 21, 2018

What I'm arguing is, can we avoid depending on packaging since it might be moot anyway.

@di
Copy link
Contributor Author

di commented Mar 21, 2018

@peterbe That's a great point, I hadn't actually looked closely at what you were using packaging for. Adding the python_requires bit should prevent incompatible platforms from installing this next release, and we can just catch the Incompatible exception from pip-api instead of this check.

Fixing...

@di
Copy link
Contributor Author

di commented Mar 21, 2018

Wait, I think I've misunderstood. I thought you were using packaging to check the pip version, but you're using it to parse the distribution version and check if it's a prerelease:

hashin/hashin.py

Lines 217 to 220 in 66aa748

for version in data['releases']:
v = parse(version)
if not v.is_prerelease:
all_versions.append((v, version))

Seems to me like you still need the packaging dependency?

@di
Copy link
Contributor Author

di commented Mar 21, 2018

This is where you're checking the pip version:

hashin/hashin.py

Lines 75 to 79 in 66aa748

major_pip_version = int(pip.__version__.split('.')[0])
if major_pip_version < 8:
raise ImportError(
'hashin only works with pip 8.x or greater'
)

In this PR, I've switched it to using pip-api as well so you don't have to import pip at all

@peterbe
Copy link
Owner

peterbe commented Mar 21, 2018

I don't know how pip-api works. But suppose I have an environment where I have:

  • python 2.7.11
  • pip 7
  • latest hashin
  • latest pip-api

In that case, will pip_api.hash(filename, algorithm) work at all? If pip7 doesn't have hashing of files into sha strings, will pip_api.hash fail horribly?

I have a feeling that pip_api.version() would return "7" in this scenario and that pip_api.hash would fail. If so be the case, this PR is ready to land.

Copy link
Owner

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

I checked the documentation for pip-api and basically, the same reality is still the case. If you have pip <8 installed hashing won't work even with latest pip-api installed. In other words, we need to keep that if major_pip_version < 8: check around except with this PR, the way we get to major_pip_version is now done differently.

@di
Copy link
Contributor Author

di commented Mar 21, 2018

Yeah, pip-api would raise an Incompatible exception. In hashin, we could either keep the existing check, or catch that exception from pip-api. I think either would be identical to the user, figured it was easiest to just leave the existing check but I'm happy to change it if you'd prefer!

@peterbe peterbe merged commit 1f794c7 into peterbe:master Mar 21, 2018
@di di deleted the use-pip-api branch March 21, 2018 18:53
@peterbe
Copy link
Owner

peterbe commented Mar 21, 2018

Just release 0.13.0
https://twitter.com/peterbe/status/976537835669999616

Thanks for all your hard work even though I failed to mention you in my tweet.

It's a bit weird to make a minor upgrade when the API didn't change at all. But this way the 2.6 and 3.3 support drop is more clear. Plus to get 0.13.0 you now also need to install other stuff.

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.

2 participants