fix for: Incorrect username/password gives misleading error #5659
Conversation
|
This CI failure is unrelated to the change...i'm not sure of why the build has timed out. |
|
CI checks are passing...can I get this merged? |
|
I won't be able to take a look at this since there's quite a bit happening offline for me currently. Perhaps some other maintainer is able to find time to look at this before I come around to it. :) Thanks and sorry for the wait. |
src/pip/_internal/download.py
Outdated
| logger.warning('401 Error, Credentials not correct for %s', | ||
| resp.request.url) | ||
| return resp | ||
|
|
There was a problem hiding this comment.
If I'm reading the Requests documentation correctly, you only need to return a value if you're retruning something different. So
if resp.status_code == 410:
logger.warning(...)should be all you need (and it looks cleaner).
src/pip/_internal/download.py
Outdated
| @@ -199,13 +199,23 @@ def handle_401(self, resp, **kwargs): | |||
|
|
|||
| # Add our new username and password to the request | |||
| req = HTTPBasicAuth(username or "", password or "")(resp.request) | |||
| req.register_hook("response", self.handle_401_again) | |||
There was a problem hiding this comment.
Minor naming nit: we're not actually handling the 401, and definitely not doing so "again". Maybe warn_on_401?
pradyunsg
added
type: enhancement
src/pip/_internal/download.py
Outdated
| if resp.status_code == 401: | ||
| logger.warning('401 Error, Credentials not correct for %s', | ||
| resp.request.url) | ||
| return resp |
|
ah, thanks, removed the return and fixed the function name to be the recommended "warn_on_401" |
|
I did not seen any existing 401 tests to leverage from. Is that required for the merge? Any other opens I need to take care of before this would be qualified to get merged in? |
|
Since this is approved, can someone merge this so that it makes it to the next release? |
|
@patter001 thanks for the PR 👍 |
|
very cool. glad to provide some small help to an awesome tool. Thanks for the merge! |
|
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. |
lock
bot
added
the
auto-locked
No description provided.