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.
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
Adding missing docstrings to
twine/commands
#799Adding missing docstrings to
twine/commands
#799Changes from 2 commits
a41131d
1a597fa
75f2083
8df1439
71d07cf
6893df2
3b15067
8383dd1
e1d4ee9
6e558eb
4b16d25
ebd6696
496cf1c
5b7c50b
2953d93
8656313
05cce04
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@meowmeowmeowcat Why was this and the docstring for
check._check_file
deleted? If anything, I'm inclined to require docstrings on private functions and methods, since they often have non-trivial logic.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 think it's because I just want to pass the
flake8
test and make the review easier, so I only add docstrings to public functions first. If you want, I can take some time and add docstrings to private functions. (Or do you prefer adding them by yourself?) :)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.
Good instinct to limit the scope, and at this point I'd rather add private docstrings in a separate PR. However, it seems like you deleted these docstrings. Were they causing flake8 to fail?
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 think they were not causing flake8 to fail. Only missing public docstrings will cause flake8 to fail.
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.
Okay. With that in mind, I've restored them.