gh-77065: Add optional keyword-only argument echo_char for getpass.getpass#130496
gh-77065: Add optional keyword-only argument echo_char for getpass.getpass#130496hugovk merged 24 commits intopython:mainfrom
echo_char for getpass.getpass#130496Conversation
|
@picnixz sorry for choosing you, I just haven't found anyone else from the core team who is active yet (I looked at who is involved in issue and experts table). could you review? |
|
You can ping me for anything that is remotely related to security and cryptography |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
@picnixz I chose |
Because if you were to use it, you would write |
Usually, you expect 1 character indeed. I don't think we should write something else than 1 char (or maybe we do but it's because of colors?) I would not expect words to be put as placeholders. Maybe colorized characters, which in this case are allowed to be strings of length > 1, but not real words. Also, what we are echoing is really the corresponding password character. For each character, we use that placeholder. |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
mask for getpass.getpassechochar for getpass.getpass
|
@picnixz I just thought maybe we also should to add |
|
@picnixz, I added test cases, what do you think about them? |
I added test case with control chars |
cc @picnixz |
|
@picnixz It seems that all the tests that you requested already exist, but maybe it will be possible to add more since there is already an issue for covering the module with tests I would like to have time to accept before feature freeze :-) |
|
I am travelling (I'm back after the beta release) so I won't merge anything on mobile. I'll defer this to @gpshead, sorry but it passed under my radar :( (in the future, a request for review is more likely to be noticed) |
Lib/getpass.py
Outdated
| return line | ||
|
|
||
|
|
||
| def _input_with_echochar(prompt, stream, input, echochar): |
There was a problem hiding this comment.
rather than duplicating so much of _raw_input's logic, I suggest making a _readline_with_echochar() function and just conditionally calling that vs the existing input.readline() call within _raw_input().
There was a problem hiding this comment.
sounds reasonable, please check my diff
echochar for getpass.getpassecho_char for getpass.getpass
|
Thank you everyone! |
…etpass.getpass` (python#130496) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
📚 Documentation preview 📚: https://cpython-previews--130496.org.readthedocs.build/