-
Notifications
You must be signed in to change notification settings - Fork 305
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
#799
Conversation
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.
Thanks for taking a first pass at this, and apologies for the lag in the review. In general, I think the docstrings should be more descriptive of what each function does and why (which I realize might be hard to determine). I don't have specific recommendations at the moment, but I can come up with some.
Also, I think any PR towards #635 or #636 should remove the relevant ignore(s) in the Flake8 configuration:
Line 16 in beecc17
twine/commands/*: D100,D101,D102,D103,D104 |
@bhrutledge Feel free to comment here once you have recommendations.
Did you mean that I should remove |
Remove them first to show that tests are failing first and then begin to pass, also to ensure that you're not going to be surprised once you do remove them |
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.
These docstrings don't seem to describe the functions any better than the function names themselves. In an ideal world, they'd help someone completely new to the code-base understand what the purpose is, why we need it, and what the behaviour is based on the arguments passed.
Note this isn't for a public API here but for internal usage as developers and maintainers. You can be as extremely technical as you'd like as that will help future users here. These are never to be used as a public API
Thanks for answering.
I'll take some time and go through these codes again for making better docstrings. Once I've done it, I'll make this PR as ready for review. Thanks for your review here! |
I am not really sure about what should I add in |
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 Thanks for adding some depth to the docstrings. I'll want to do some copy-editing before merge, but there's a style question that I'd like to resolve first.
Co-authored-by: Ian Stapleton Cordasco <graffatcolmingov@gmail.com>
…eowcat/twine into add-missing-docstrings
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 Since apologies for the lag on this. As evidenced by #810, I went down a bit of a rabbit hole.
I pushed a bunch of changes that reformat the docstrings for Sphinx, but also rewrote them in a way that believe adds clarity. In general, I focused on "what" a function is doing, rather than "how", and relied on the :param:
and :return:
directives to add details.
Feedback welcome, especially from @sigmavirus24, who I think has a lot more experience in this area.
twine/commands/upload.py
Outdated
@@ -57,7 +72,6 @@ def skip_upload( | |||
def _make_package( | |||
filename: str, signatures: Dict[str, str], upload_settings: settings.Settings | |||
) -> package_file.PackageFile: | |||
"""Create and sign a package, based off of filename, signatures and settings.""" |
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.
However, it seems like you deleted these docstrings. Were they causing flake8 to fail?
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.
An example of the rendered API docs can be seen at https://twine--810.org.readthedocs.build/en/810/api/twine.commands.html#submodules. |
@sigmavirus24 Any feedback before I merge this?
|
Part of #635.