Warn if an invalid URL is passed with --index-url

[deveshks]

Fixes and Closes #7430

The idea was taken from https://github.com/pypa/pip/blob/master/src/pip/_vendor/distlib/util.py#L192

$ pip install --index-url --user https://test.pypi.org/simple/ reretrieve
WARNING: index-url --user is invalid.
Collecting https://test.pypi.org/simple/
  Using cached https://test.pypi.org/simple/ (3.4 MB)
  ERROR: Cannot unpack file /private/var/folders/xg/blp845_s0xn093dyrtgy936h0000gp/T/pip-unpack-82nw6u8k/simple (downloaded from /private/var/folders/xg/blp845_s0xn093dyrtgy936h0000gp/T/pip-req-build-w612tjr_, content-type: text/html; charset=UTF-8); cannot detect archive format
ERROR: Cannot determine archive format of /private/var/folders/xg/blp845_s0xn093dyrtgy936h0000gp/T/pip-req-build-w612tjr_

[SalomonSmeke]

Thank you very much for working on this!

[SalomonSmeke]

One question: from a comment on that ticket, it seems index-url can point to directories. Is that the case? How does that factor here?

[deveshks]

One question: from a comment on that ticket, it seems index-url can point to directories. Is that the case? How does that factor here?

Hi @SalomonSmeke ,

Thanks for the comment, the document on --index-url does state that the url should point to a repository compliant with PEP 503 (the simple repository API) or a local directory laid out in the same format.

So it pointing to a local directory might cause a warning in my current case, unless I explicitly check if the url is a valid local directory path. I will add that check using os.path.exists perhaps.

On another thought, I think that even for a local directory, we have to use a file: scheme, so this test would still work. and we also have a check here https://github.com/pypa/pip/blob/master/src/pip/_internal/vcs/versioncontrol.py#L46 which I see doesn't get hit if I remove my check.

[SalomonSmeke]

@deveshks thank you very much for checking!

[deveshks]

No worries @SalomonSmeke .

Hi @uranusjr ,

As per your comment , #7430 (comment) , I have created this PR to handle cases with invalid --index-url arguments. Please take a look whenever feasible :)

[deveshks]

Hi @uranusjr ,

I have made the requested changes in the PR. Please take a look :)

[uranusjr]

One more comment, otherwise LGTM

[uranusjr]

Oh and also, remember the news fragment.

[deveshks]

Oh and also, remember the news fragment.

Hi @uranusjr ,

I have already added the news fragment. Is there an issue with the content or the file extension, which is .bugfix now?

[uranusjr]

@deveshks Sorry, I mis-read the diff. The news fragment is fine 🙂

[deveshks]

@deveshks Sorry, I mis-read the diff. The news fragment is fine 🙂

Thanks @uranusjr.

Also for your earlier about not needing self.index_urls, please refer to #7965 (comment) as well.

Please let me know if there are any other concerns 🙂

[deveshks]

Thanks @uranusjr for the approval.

Hi @pradyunsg , could I please get this PR merged as well 😊

[McSinyx]

More of a question than a review, but what's pip's position to newline use recommended by PEP 8?

[pfmoore]

@McSinyx The lint jobs handle that. We have a flake8 config somewhere, but to be honest I personally just tend to follow the surrounding code as best I can,. then run lint and fix whatever it complains about.

(IIRC, it's 2 blank lines between top-level definitions, 1 blank line between definitions in a class, and must have a newline at the end of the file).

[McSinyx]

@pfmoore, sorry for not phrasing the question clearly enough. I was referring to this part of PEP 8:

Use blank lines in functions, sparingly, to indicate logical sections.

Since logical section is totally up to us to decide, within pip codebase I see many spots where newlines are used deliberately every other line and wonder if we want to improve that. The reasoning (too bad it's not in the PEP) is since methods are separated by only one blank line, overuse of newlines within a method can make it a bit harder to follow.

[deveshks]

Hi @uranusjr

I forgot about it earlier, but I have also added a test case to verify the behaviour for the change in the PR. Please look at the same as well 😊

[pfmoore]

@McSinyx Ah right, I understand what you mean. My personal view would be that if you're changing the code, then tidying up line spacing is fine, although you should be aware that "readability" is very personal, so err on the side of caution.

I would not recommend doing a larger "tidying up" pass on the code. It tends to cause merge conflicts in other PRs, and generally do more harm than good.

[McSinyx]

@deveshks, there's a lint issue. Edit: seems like CI issue 😞

[deveshks]

@deveshks, there's a lint issue. Edit: seems like CI issue 😞

Thanks @McSinyx for the heads up. Closing/Reopening the PR didn't help, so I had to force-pushed a commit 😞 Hopefully the checks should pass now.

[McSinyx]

It's passing now. I wonder if we can lower the number of CIs to avoid false alarms like this.

[deveshks]

Thanks @uranusjr for the approval.

Hi @xavfernandez , @pradyunsg ,

If the changes look good, could I please get this PR merged as well 😊