-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add --exit-code
option and add number of files to output
#81
Conversation
Pull Request Test Coverage Report for Build 2027370813
💛 - Coveralls |
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.
Looks great ! I have two small suggestions but feel free to disregard.
pydocstringformatter/run.py
Outdated
if not is_changed: # pylint: disable=consider-using-assignment-expr | ||
if len(filepaths) > 1: | ||
files_string = f"{len(filepaths)} files" | ||
else: | ||
files_string = "1 file" | ||
|
||
utils._print_to_console( | ||
f"Nothing to do! All docstrings in {files_string} are correct 🎉\n", | ||
self.config.quiet, | ||
) | ||
utils._sys_exit(0, self.config.exit_code) | ||
else: | ||
utils._sys_exit(32, self.config.exit_code) |
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.
if not is_changed: # pylint: disable=consider-using-assignment-expr | |
if len(filepaths) > 1: | |
files_string = f"{len(filepaths)} files" | |
else: | |
files_string = "1 file" | |
utils._print_to_console( | |
f"Nothing to do! All docstrings in {files_string} are correct 🎉\n", | |
self.config.quiet, | |
) | |
utils._sys_exit(0, self.config.exit_code) | |
else: | |
utils._sys_exit(32, self.config.exit_code) | |
if is_changed: # pylint: disable=consider-using-assignment-expr | |
utils._sys_exit(32, self.config.exit_code) | |
return # We may not sys exit depending on options | |
files_string = f"{len(filepaths)}" | |
files_string += "files" if len(filepaths) > 1 else "file" | |
utils._print_to_console( | |
f"Nothing to do! All docstrings in {files_string} are correct 🎉\n", | |
self.config.quiet, | |
) | |
utils._sys_exit(0, self.config.exit_code) |
You know my favorite thing in the world is premature return I just had to say it 😄 Depending on what you think of displaying the name of a file if it's unique there the suggestion might need some tweaking.
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 changed it around a little bit. You actually found a small bug: for 0 files
checked due to exclude patterns we still said that 1 file was correct.
I changed it (and fixed the tests).
pydocstringformatter/run.py
Outdated
if len(filepaths) > 1: | ||
files_string = f"{len(filepaths)} files" | ||
else: | ||
files_string = "1 file" |
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.
files_string = "1 file" | |
files_string = f"'{filepaths[0].name}'" |
Maybe a nice little bonus ?
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.
Fails with IndexError
in some test cases. I'll keep it in mind for a future enhancement.
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.
👍
Closes #4.
Also moved some stuff around and updated the output a little.
I kept the proposed exit codes as it just makes everything future proof.
For future reference:
0 - Nothing happened, all files are fine.
1 - Internal error
2 - Config parsing error
4, 8, 16 - Empty for future use
32 - A file was changed or could be changed
64, 128, 256 - Empty for future use