-
Notifications
You must be signed in to change notification settings - Fork 307
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
Show more helpful messages for invalid passwords #815
Changes from 4 commits
f60db37
cd2d5f5
b640db9
0b5b625
d108490
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
import logging | ||
import os | ||
import os.path | ||
import unicodedata | ||
from typing import Any, Callable, DefaultDict, Dict, Optional, Sequence, Union | ||
from urllib.parse import urlparse | ||
from urllib.parse import urlunparse | ||
|
@@ -237,11 +238,31 @@ def get_userpass_value( | |
if cli_value is not None: | ||
logger.info(f"{key} set by command options") | ||
return cli_value | ||
|
||
elif config.get(key) is not None: | ||
logger.info(f"{key} set from config file") | ||
return config[key] | ||
|
||
elif prompt_strategy: | ||
return prompt_strategy() | ||
warning = "" | ||
value = prompt_strategy() | ||
|
||
if not value: | ||
warning = f"Your {key} is empty" | ||
elif any(unicodedata.category(c).startswith("C") for c in value): | ||
# See https://www.unicode.org/reports/tr44/#General_Category_Values | ||
# Most common case is "\x16" when pasting in Windows Command Prompt | ||
warning = f"Your {key} contains control characters" | ||
|
||
if warning: | ||
logger.warning(f" {warning}. Did you enter it correctly?") | ||
logger.warning( | ||
" See https://twine.readthedocs.io/#entering-credentials " | ||
"for more information." | ||
) | ||
|
||
return value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sigmavirus24 What do you think of something like this? I haven't tried it on Windows (yet), but here's what it looks like with an empty password:
If this seems reasonable, I'm happy to add a new section to Twine's docs, to keep this independent of a specific repository. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM |
||
|
||
else: | ||
return None | ||
|
||
|
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.
@sigmavirus24 I added this note, and updated the code to it. What do you think?
https://twine--815.org.readthedocs.build/en/815/#using-twine
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 might move this up and indent it underneath item 2, but that's more of a nit
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 did that at first, but it felt distracting. I also think it works better as a direct link outside of the steps.