-
Notifications
You must be signed in to change notification settings - Fork 726
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
Hide password during typing with a new prompt input #1962
Open
Crissal1995
wants to merge
9
commits into
prompt-toolkit:master
Choose a base branch
from
Crissal1995:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
12239da
Implemented the main functionality
Crissal1995 979ea9c
add some tests
Crissal1995 c2e9da5
modified and tested the example
Crissal1995 78b83a1
ruff format
Crissal1995 47a7a57
mypy fix
Crissal1995 901db37
Update tests/test_cli.py
Crissal1995 3790a61
moved common instruction to method
Crissal1995 901add0
found the bug, unrelated from the change
Crissal1995 fba31a1
removed comment not applicable anymore
Crissal1995 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Would it make sense to accept
password_character
here instead ofhide_password
? It will be more flexible.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.
Undoubtedly, but from an user perspective
hide_password
is more clear.Besides, is there any valuable use-case where the password is displayed as something else, e.g.
xxxxx
or11111
?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 can imagine that some might want to render it as Unicode bullets, because that's more beautiful.
Like
password_character="•"
I mainly want to avoid having too many redundant arguments here. We have to repeat them in multiple places and the signature gets pretty long. If at some point, some users want to configure this, we'll have both
hide_password
andpassword_character
.Honestly, I'm not sure myself. If there was some prior art in other libraries, that could convince me to go one way or another.
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.
An alternative would be to use both flags, forcing
password_character
to be an empty string ifhide_password
isTrue
.In this way users could either use the flag
hide_password
if they don't care about how the password is displayed on screen, or the flagpassword_character
if they want to customize the appearance, as youy suggested.