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

Display URL downloading from #2183

Merged
merged 1 commit into from Dec 18, 2014

Conversation

Projects
None yet
4 participants
@msabramo
Contributor

msabramo commented Dec 14, 2014

Display URL downloading from instead of just filename when using index other than PyPI.

It's useful to distinguish between downloading from PyPI or from an internal devpi server, for example. In the latter case, it is useful to see the full URL, to know which index pip is downloading from.

E.g.:

Downloading from PyPI is unchanged:

$ pip install --no-cache-dir --ignore-installed Jinja2
...
  Downloading Jinja2-2.7.3.tar.gz (378kB)

But downloading from a different server results in displaying the full URL:

$ pip install --no-cache-dir --ignore-installed -i http://mirror.picosecond.org/pypi/simple jinja2
...
  Downloading http://mirror.picosecond.org/pypi/packages/source/J/Jinja2/Jinja2-2.7.3.tar.gz (378kB)
@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo
Contributor

msabramo commented Dec 14, 2014

@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo
Contributor

msabramo commented Dec 14, 2014

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Dec 15, 2014

Contributor

I'm surprised "pypi.python.org" is not a constant defined somewhere in pip's code, just like distlib seems to do:
https://github.com/pypa/pip/blob/develop/pip/_vendor/distlib/index.py#L25

Contributor

xavfernandez commented Dec 15, 2014

I'm surprised "pypi.python.org" is not a constant defined somewhere in pip's code, just like distlib seems to do:
https://github.com/pypa/pip/blob/develop/pip/_vendor/distlib/index.py#L25

@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Dec 16, 2014

Contributor

I'm surprised "pypi.python.org" is not a constant defined somewhere in pip's code

See #2199

Contributor

msabramo commented Dec 16, 2014

I'm surprised "pypi.python.org" is not a constant defined somewhere in pip's code

See #2199

@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Dec 16, 2014

Contributor

@dstufft, @pfmoore: What do you guys think?

Contributor

msabramo commented Dec 16, 2014

@dstufft, @pfmoore: What do you guys think?

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Dec 16, 2014

Member

I presume this will need changing when #2199 lands?

It's a bit verbose, and I'd like to check out how it interacts with --find-links (I'd rather it didn't show a file: URL for those). It might be nicer if it said something like

Downloading Jinja2-2.7.3.tar.gz from http://mirror.picosecond.org/pypi/simple (378kB)

but I appreciate that may well be harder to do (as we may no longer have the original --index-url/--find-links argument at this point in the code). But I agree with the principle of showing where the file came from.

Member

pfmoore commented Dec 16, 2014

I presume this will need changing when #2199 lands?

It's a bit verbose, and I'd like to check out how it interacts with --find-links (I'd rather it didn't show a file: URL for those). It might be nicer if it said something like

Downloading Jinja2-2.7.3.tar.gz from http://mirror.picosecond.org/pypi/simple (378kB)

but I appreciate that may well be harder to do (as we may no longer have the original --index-url/--find-links argument at this point in the code). But I agree with the principle of showing where the file came from.

@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Dec 17, 2014

Contributor

I presume this will need changing when #2199 lands?

I don't know of any changes that it needs now that #2199 has landed -- were you expecting merge conflicts or do you see something that needs to be changed?

I'd like to check out how it interacts with --find-links (I'd rather it didn't show a file: URL for those)

I don't think this has any interaction with --find-links as it's only displayed when something is downloaded, so I'd never expect to see a file: URL.

I appreciate that may well be harder to do (as we may no longer have the original --index-url/--find-links argument at this point in the code).

Yeah, we don't have that information so I don't know how to do that without bigger changes.

Contributor

msabramo commented Dec 17, 2014

I presume this will need changing when #2199 lands?

I don't know of any changes that it needs now that #2199 has landed -- were you expecting merge conflicts or do you see something that needs to be changed?

I'd like to check out how it interacts with --find-links (I'd rather it didn't show a file: URL for those)

I don't think this has any interaction with --find-links as it's only displayed when something is downloaded, so I'd never expect to see a file: URL.

I appreciate that may well be harder to do (as we may no longer have the original --index-url/--find-links argument at this point in the code).

Yeah, we don't have that information so I don't know how to do that without bigger changes.

Show outdated Hide outdated pip/download.py Outdated
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Dec 17, 2014

Member

I don't think this has any interaction with --find-links as it's only
displayed when something is downloaded, so I'd never expect to see a file:
URL.

OK, if that's the case then that's fine.

I appreciate that may well be harder to do (as we may no longer have the
original --index-url/--find-links argument at this point in the code).

Yeah, we don't have that information so I don't know how to do that
without bigger changes.

Fair enough. The form in the PR looks fine in that case.

Member

pfmoore commented Dec 17, 2014

I don't think this has any interaction with --find-links as it's only
displayed when something is downloaded, so I'd never expect to see a file:
URL.

OK, if that's the case then that's fine.

I appreciate that may well be harder to do (as we may no longer have the
original --index-url/--find-links argument at this point in the code).

Yeah, we don't have that information so I don't know how to do that
without bigger changes.

Fair enough. The form in the PR looks fine in that case.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Dec 17, 2014

Member

OK. I'll leave it till tomorrow in case you come up with any other ideas, or someone else wants to chip in. But if there's no further comments, I'll merge tomorrow (assuming the tests finish OK - py3.2 I love you...). Ping me if I forget.

Member

pfmoore commented Dec 17, 2014

OK. I'll leave it till tomorrow in case you come up with any other ideas, or someone else wants to chip in. But if there's no further comments, I'll merge tomorrow (assuming the tests finish OK - py3.2 I love you...). Ping me if I forget.

Display URL downloading from
instead of just filename when using index other than PyPI.

It's useful to distinguish between downloading from PyPI or from an
internal devpi server, for example. In the latter case, it is useful to
see the full URL, to know which index pip is downloading from.

E.g.:

Downloading from PyPI is unchanged:

    $ pip install --no-cache-dir --ignore-installed Jinja2
    ...
      Downloading Jinja2-2.7.3.tar.gz (378kB)

But downloading from a different server results in displaying the full
URL:

    $ pip install --no-cache-dir --ignore-installed -i http://mirror.picosecond.org/pypi/simple jinja2
    ...
      Downloading http://mirror.picosecond.org/pypi/packages/source/J/Jinja2/Jinja2-2.7.3.tar.gz (378kB)
@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Dec 18, 2014

Contributor

I did end up revising a bit.

Tests passed: https://travis-ci.org/pypa/pip/builds/44396166

What I did is move the new Index class and PyPI instance from pip/index.py (which is the largest file in pip and used by a lot of modules) to pip/models/index.py in a new pip.models package. Code can depend on this without having yet another dependency on pip.index.

Contributor

msabramo commented Dec 18, 2014

I did end up revising a bit.

Tests passed: https://travis-ci.org/pypa/pip/builds/44396166

What I did is move the new Index class and PyPI instance from pip/index.py (which is the largest file in pip and used by a lot of modules) to pip/models/index.py in a new pip.models package. Code can depend on this without having yet another dependency on pip.index.

@@ -0,0 +1 @@
from pip.models.index import Index, PyPI # noqa

This comment has been minimized.

@dstufft

dstufft Dec 18, 2014

Member

Instead of a # noqa here I'd prefer to define an __all__ that has Index and PyPI.

@dstufft

dstufft Dec 18, 2014

Member

Instead of a # noqa here I'd prefer to define an __all__ that has Index and PyPI.

This comment has been minimized.

@dstufft

dstufft Dec 18, 2014

Member

Nevermind, I'll just push a commit after merge.

@dstufft

dstufft Dec 18, 2014

Member

Nevermind, I'll just push a commit after merge.

dstufft added a commit that referenced this pull request Dec 18, 2014

@dstufft dstufft merged commit 0393ea8 into pypa:develop Dec 18, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment