Adding missing docstrings to twine/commands
#799
Conversation
There was a problem hiding this comment.
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
@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.
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 |
Co-authored-by: Ian Stapleton Cordasco <graffatcolmingov@gmail.com>
…eowcat/twine into add-missing-docstrings
There was a problem hiding this comment.
@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.
@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.
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.
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.
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.
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.