Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

The 'port' command returns a package name instead of the original link. #116

Merged
merged 1 commit into from
Feb 16, 2016
Merged

The 'port' command returns a package name instead of the original link. #116

merged 1 commit into from
Feb 16, 2016

Conversation

jotes
Copy link
Contributor

@jotes jotes commented Feb 1, 2016

Hi @erikrose,
During our migration from peep to pip 8 i've discovered that peep port can't port requirements that rely on the urls to the remote resources.
Could you review my patch?
Unfortunately i couldn't find any tests for peep port but I can add them if you wish so.

Thanks for your hard work on peep and pip8!

@erikrose
Copy link
Owner

erikrose commented Feb 1, 2016

Hi; thanks for the patch! The peep port tests are over here, and it should be pretty easy for you to add one: https://github.com/erikrose/peep/blob/master/tests/__init__.py#L374.

@@ -920,7 +920,11 @@ def peep_port(paths):
if not hashes:
print(req.req)
else:
print('%s' % req.req, end='')
if req.original_link:
Copy link
Owner

Choose a reason for hiding this comment

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

IIRC, I added the original_link attr only in pip 8, so this will crash in older versions. Also req_string isn't a string in the case of req.req. One way we could fix both is by replacing this whole if with an inline ternary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erikrose I'm sorry it took so log, but i had to skim through a lot of pip versions in order to make me more confident regarding this changes.
It looks like at some point, urls were stored in a property called url. However, contents of this property were inconsistent between versions of pip, that's why decided to support only versions above 6.1.0. Is that okay with you? Or do you have any suggestions about direction i should take?
Tests should pass from now on.

@edmorley
Copy link
Contributor

Other than the small tweak above, this looks good to me.
@erikrose, are you happy for me to merge this once that's changed?

@jotes
Copy link
Contributor Author

jotes commented Feb 12, 2016

hi @edmorley, could you review my PR again?

@erikrose
Copy link
Owner

FWIW, I think using link rather than original_link here should be okay, since we won't have called _prepare_requirements() yet, which is what can replace link with a computed value (the download address of a package that was specified by name).

@jotes
Copy link
Contributor Author

jotes commented Feb 16, 2016

@erikrose I totally agree. Is there anything I can do to help you with this PR?

@erikrose
Copy link
Owner

I'm going to have a look at it later today and probably merge it in (if @edmorley doesn't get there first), then do a release.

erikrose added a commit that referenced this pull request Feb 16, 2016
The 'port' command returns a package name instead of the original link.
@erikrose erikrose merged commit 55a0cf7 into erikrose:master Feb 16, 2016
@jotes
Copy link
Contributor Author

jotes commented Feb 16, 2016

@erikrose Awesome, thanks!

@jotes jotes deleted the fix_broken_urls branch July 3, 2019 09:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants