-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix #5889: AttributeError: 'NoneType' object has no attribute 'netloc' #6336
Conversation
Thanks for the PR.
Well, while the change is trivial, it is still a bugfix for #5889 and deserve an entry in the change log... |
I'm on it.
I'm on it too. |
Hi @xavfernandez, Thank you. |
@aloosley yes please.
|
Doing this just tells me the same as the CI, that
|
indicates the missing final
indicates that this line should move right after FWIW, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few random comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjerdonek , thanks for comments on the code. I hope they are all solved now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your work 👍
Thanks also for the intro to isort / tox. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nits before merging.
Done. |
tests/unit/test_req_install.py
Outdated
when called with URL (PEP 508) but without comes_from. | ||
""" | ||
# Test with a PEP 508 url install string: | ||
wheel_url = ("https://download.pytorch.org/whl/cu90/" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're going to get a style error here, but note that you don't need a +
(parentheses concatenate without that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might need to do something like the following to address linter complaints--
wheel_url = (
"https://download.pytorch.org/whl/cu90/..."
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now done?
Thanks a lot @aloosley 🎉 |
Is this fix in a released pip yet? Or perhaps there is a conda channel I can use to double-check this fix? Looking at the test, I don't think it properly exercises the bug I originally reported. Also, as a side-note. I am not familiar with pip's testing system, but it looks like the test added here depends upon an external resource? Why not use a fixture approach instead of fetching from the internet? |
No, but it will be in the next release (19.1).
It looks fine to me. For example, without the code change, one of the added tests will raise:
It doesn't look to me like it fetches from the internet. It just calls |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Hello,
If accepted, this trivial change solves the issue I started to describe March 14 2019 in issue 5889. To be self contained, I elaborate everything below:
I am installing a private package called
deep-nlp-embedding
(pip install -e .
) that depends on another private package calleddeep-nlp-core
that was previously installed withpip install -e .
The package
deep-nlp-core
has install_requires:The package
deep-nlp-embeddings
has install_requires:Finally installing deep-nlp-embedding breaks pip:
Because this change is trivial, I assume it doesn't require a
NEWS.rst
entry.