Improve redirect error message #460
Conversation
Codecov Report
@@ Coverage Diff @@
## master #460 +/- ##
==========================================
+ Coverage 80.74% 82.93% +2.18%
==========================================
Files 14 14
Lines 748 750 +2
Branches 108 108
==========================================
+ Hits 604 622 +18
+ Misses 110 92 -18
- Partials 34 36 +2
Continue to review full report at Codecov.
|
twine/exceptions.py
Outdated
| @classmethod | ||
| def from_args(cls, repository_url, redirect_url): | ||
| msg = "\n".join([ | ||
| "Unexpected redirect from {0} to {1}." |
There was a problem hiding this comment.
I don't think we support 2.6 any longer meaning you can drop the numbers here, if you'd like.
twine/exceptions.py
Outdated
| msg = "\n".join([ | ||
| "Unexpected redirect from {0} to {1}." | ||
| .format(repository_url, redirect_url), | ||
| "You might need to change the configured repository URL.", |
There was a problem hiding this comment.
In the more common cases a redirect here is innocuous and perhaps changing the URL is fine. But there could be cases where folks are being redirected to a malicious URL that will be able to:
- steal credentials
- store the uploaded package data
In that case, I don't want folks to think "Geeze, really?! I have to copy and paste this into my pypirc for this stupid tool to work?! WTF IS WRONG WITH THOSE PEOPLE" and then to angrily and carelessly copy and paste the malicious URL.
Granted, I think the this is very unlikely, but still I would like to see more cautious wording. Something like:
You should examine the URLs carefully and if you trust them, update your configured repository URL.
Also, would you share your reasoning for using a newline to separate these lines? Has the project moved towards preferring newlines?
There was a problem hiding this comment.
|
@sigmavirus24 Is there more that I can/should do with this pull request? |
There was a problem hiding this comment.
Sorry for the delay @bhrutledge I'm not really spending time on F/OSS and it was purely chance that I noticed this in my email today. It may be best to work with another maintainer on this going forward.
tests/test_upload.py
Outdated
| upload.upload(upload_settings, [WHEEL_FIXTURE]) | ||
|
|
||
| assert err.value.args[0] == ( | ||
| "https://test.pypi.org/legacy attempted to redirect" |
There was a problem hiding this comment.
This is something I struggle with often. As a CLI, our user interface is what we print out. With that in mind, tests like these quickly become frustrating and feel overly strict when updating messages/message formatting/etc. I think testing that we're showing the correct data is important, but I also don't want us to test that we always show this message. I can imagine a future where we use internationalization to improve the experience and someone contributing from a locale that isn't en_* sees a test failure here too.
What do you think are the important parts of this message? What should we test for and what can we eliminate from this assertion?
There was a problem hiding this comment.
Thanks, @sigmavirus24. In this case, I followed the pattern of the existing tests, but in generally I agree with your opinion. To that end, in f326758 I modified the assertions to check for what I think are the important bits, like URLs.
|
@theacodes or @di: in light of this comment from @sigmavirus24, and as the other maintainers that I've interacted with, any thoughts on this PR?
|
|
Just a heads up, I'm not technically a twine maintainer, but @theacodes or @jaraco are. |
Closes #310
RedirectDetectederror message inuploadregisterto use the new message (with a test)conftest.pyfor reused fixturesThe original issue has a suggestion to avoid the redirect as a special case for PyPI. I would probably do that in
utils.normalize_repository_urlby appending a trailing slash if the URL is in_HOSTNAMES. However, it seemed like a more useful error message would benefit other repositories, and might be sufficient for the official ones.