Skip to content
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
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Crissal1995
Copy link

This PR resolves #1783.
The PasswordProcessor by default (current implementation, no argument given) outputs a * for each letter typed by the user in the prompt.
However, the desired behavior would be to make the typed password invisible, e.g. like getpass does.
Two tests were added, and the example was also modified and tested locally to make sure that the change works as intended.

New input added to the prompt(): hide_password
False by default, if True, the characters won't be echoed back (as *) in the output
@@ -249,6 +249,7 @@ class PromptSession(Generic[_T]):
When True (the default), automatically wrap long lines instead of
scrolling horizontally.
:param is_password: Show asterisks instead of the actual typed characters.
:param hide_password: Hide the password, rather than showing asterisks.
Copy link
Member

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 of hide_password? It will be more flexible.

Copy link
Author

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 or 11111?

Copy link
Member

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 and password_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.

Copy link
Author

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 if hide_password is True.
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 flag password_character if they want to customize the appearance, as youy suggested.

tests/test_cli.py Outdated Show resolved Hide resolved
@Crissal1995
Copy link
Author

Crissal1995 commented Jan 20, 2025

Apparently running the tests with pytest leads to a different result (green testsuite) than the one obtained by the command used by the CI pipeline, coverage run -m pytest (red testsuite); I am not sure why this is the case.

EDIT: Found an unrelated bug related to the Prompt - or the PlainTextOutput? - where the output is duplicated when it should not be.
The test is now handling this bad-weather case scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide password instead of replacing it with a character
2 participants