-
Notifications
You must be signed in to change notification settings - Fork 680
refactor: adopt EAFP and explicit error handling for password file #1203
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
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -85,6 +85,9 @@ | |
|
|
||
| SUPPORT_INFO = "Home: http://mycli.net\n" "Bug tracker: https://github.com/dbcli/mycli/issues" | ||
|
|
||
| class PasswordFileError(Exception): | ||
| """Base exception for errors related to reading password files.""" | ||
| pass | ||
|
|
||
| class MyCli(object): | ||
| default_prompt = "\\t \\u@\\h:\\d> " | ||
|
|
@@ -536,14 +539,21 @@ def _connect(): | |
| sys.exit(1) | ||
|
|
||
| def get_password_from_file(self, password_file): | ||
| password_from_file = None | ||
| if password_file: | ||
| if (os.path.isfile(password_file) or stat.S_ISFIFO(os.stat(password_file).st_mode)) and os.access(password_file, os.R_OK): | ||
| try: | ||
| with open(password_file) as fp: | ||
| password_from_file = fp.readline() | ||
| password_from_file = password_from_file.rstrip().lstrip() | ||
|
|
||
| return password_from_file | ||
| password = fp.readline().strip() | ||
| if not password: | ||
| raise PasswordFileError(f"Password file '{password_file}' is empty or contains only whitespace") | ||
| return password | ||
| except FileNotFoundError: | ||
| raise PasswordFileError(f"Password file '{password_file}' not found") from None | ||
| except PermissionError: | ||
| raise PasswordFileError(f"Permission denied reading password file '{password_file}'") from None | ||
| except IsADirectoryError: | ||
| raise PasswordFileError(f"Path '{password_file}' is a directory, not a file") from None | ||
| except OSError as e: | ||
|
||
| raise PasswordFileError(f"Error reading password file '{password_file}': {str(e)}") from None | ||
|
|
||
| def handle_editor_command(self, text): | ||
| r"""Editor command is any query that is prefixed or suffixed by a '\e'. | ||
|
|
||
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.
Doesn't this introduce a new constraint?
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.
Are you referring to
if not? If so, I thought it makes sense to raise an error when the content is emptyThere 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.
Yes, raising an error when the password is empty changes the functionality, which I suppose we should hold constant for this PR. Looking closer, I would even
strip()the newline more precisely but again that is out of scope.Because the empty string is a valid password, as are passwords containing whitespace. Whether they are advisable is a separate Q.
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.
Makes sense, I updated the code.