-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add implied assertion in WheelInfo #145
Conversation
Thanks! |
Looks like this gets hit in many tests though. |
Hum, I'll dive into it more. |
fbb7c55
to
ab71634
Compare
Ok, some of the conftest.py where mocking the url without https, let's see. |
@@ -257,7 +257,7 @@ def _mock_fetch_bytes(arg, *args, **kwargs): | |||
|
|||
msg = "Access-Control-Allow-Origin" | |||
with pytest.raises(ValueError, match=msg): | |||
await micropip.install("htps://x.com/xxx-1.0.0-py3-none-any.whl") |
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.
It's not entirely clear if the typo here was on purpose.
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.
Looks pretty accidental to me. If it were on purpose you would expect a comment.
Worse case I'll push a better error in |
@@ -7,17 +7,21 @@ | |||
"path", | |||
[ | |||
SNOWBALL_WHEEL, | |||
f"/{SNOWBALL_WHEEL}" f"a/{SNOWBALL_WHEEL}", |
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.
I think this is a mistake, and a ,
is needed.
@pytest.mark.parametrize("protocol", ["https:", "file:", "emfs:", ""]) | ||
@pytest.mark.parametrize( | ||
"protocol", | ||
["http:", "https:", "file:", "emfs:", ""], |
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.
I have no clue what emfs is..
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.
It stands for "Emscripten file system". It's needed because file:
points to files in the actual file system. Probably could use some comments and better documentation.
tests/test_transaction.py
Outdated
def test_parse_wheel_url1(protocol, path): | ||
pytest.importorskip("packaging") | ||
from micropip.transaction import WheelInfo | ||
|
||
url = protocol + path | ||
url = protocol + path if protocol else "file:///" + path |
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.
Why not put file:///
in the parametrize decorator and do:
url = protocol + path if protocol else "file:///" + path | |
url = protocol + path |
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.
It's me I'm dumb I need to put that lower; I want to check that WheelInfo.from_url(url)
parses things that have no protocol as local files.
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 for the maintenance work @Carreau! I have one suggestion otherwise lgtm.
Some simple indexes (anaconda), only list the full-pathname of download URL (assuming same domain, which make sens). WheelInfo.download does not work on this case at it tries to make the requests on the current page domain. Thus we implicitely assume that URL start with http (well https:// would be better, but when you prototype local index could be http.) The error can be weird if we let it propagate (bad ziplife as you try to decode often a 404 html page as ZIP). This thus just add an assert at construction time to catch the error early and that we start with actual protocol. It should in the end be pushed earler in the code (likely at parsing time), where we are likely to know the index URL and be able to resolve URLs at that time. I think there was also a missing comma in test parametrisations, and I added a !r in a few places as I had some error with empty filenames, without quotes, so hard to see. The conftest was also update to explicitely pass a fake URL instead of just a filename. I'm not entirely sure we should allow constructing URLs with things that don't have a protocol though, as this can be confusing.
Some simple indexes (anaconda), only list the full-pathname of download URL (assuming same domain, which make sens).
WheelInfo.download does not work on this case at it tries to make the requests on the current page domain.
Thus we implicitely assume that URL start with http (well https:// would be better, but when you prototype local index could be http.)
The error can be weird if we let it propagate (bad ziplife as you try to decode often a 404 html page as ZIP).
This thus just add an assert at construction time to catch the error early.
It should in the end be pushed earler in the code (likely at parsing time), where we are likely to know the index URL and be able to resolve URLs at that time.