Enforce PEP 257 conventions in ftplib.py #15604
Conversation
Enforce PEP 257 conventions
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
There was a problem hiding this comment.
Hello @alanyee thanks for your contribution.
IMHO this PR does not represent an improve on cpython (sorrr). I think will be complicated
review whole the code on cpython to follow the conventions. Maybe the new code
should be follows the conventions, but not the old code.
But, this is my opinion. I would like to know the core devs opinions :-)
|
Don't forget CLA @alanyee |
|
@eamanu I have signed it, though it says that it will take a business day for it to show up online. I am willing to review the code on my own, slowly transforming the code to convention. I don't think such an effort will bother other efforts. |
There was a problem hiding this comment.
Thanks for the PR @alanyee and welcome!
Although this is modifying a .py file, I'll label this as a documentation change since it's modifying comments and docstrings. I agree with the changes made, but it looks like the CI builds are failing due to whitespace issues. This can be fixed by using make patchcheck locally (or on Windows, python.bat Tools\scripts\patchcheck.py, see the devguide for more information.
Also, with regards to the CLA, it should updated in approx 24 hours after it has been signed and after an account on https://bugs.python.org has been created. Sometimes it is needed to manually refresh the status on GitHub by using the CLA heroku app.
Let me know if you have any questions. (:
By reviewing the code, I believe @eamanu was referring to the review process which has to be done by core developers before the PR can be merged. It might take a bit of time for them to get around to fully reviewing this PR, but having a proper docstring and formatting for anything in the public API does provide some value. Using the correct formatting (as opposed to an inline comment above the object's definition) allows for the usage of |
|
Agreed with @eamanu . In general convention only changes are not merged but it would be good if new code follows the convention. This also causes git blame to be less useful. Also I am not sure of using code comments as docstrings since the tone seems to vary. |
Good point. It would be significantly more helpful to work on converting some of the existing older code comments into appropriate docstrings which provide a succinct summary of the object's purpose. As the docstrings are created, the appropriate formatting could be applied. However, this would have to be done across multiple PRs (one for each object preferably) rather than an entire file at once. Edit: For clarification, this is not suggesting the current PR wouldn't be a valid change, just that creating dedicated docstrings would be especially helpful rather than just directly using the existing comments as docstrings. |
Sorry if I don't explain correctly. I mean that for example (just an example) try to fix the pep8 into the aprox 7000 files on cpython project can be complicated. Thinking about this well, it would be great for new contributor, isn't? A good opportunity to learn the workflow on cpython project. :-) |
Those were my thoughts as well. Very frequently, we've merged fairly minor documentation related changes into CPython that provided new contributors with a valuable experience. Take a look at my first merged PR as an example: #14069. Although it fixed the grammar in the section, it could have definitely been passed up as an overly trivial change that ultimately provided minimal benefit to readers of the documentation. It's important that we allow new contributors especially to make these types of PRs, in order to gain experience working within the workflow. They serve as a fantastic learning experience, not only for working within CPython, but also for contributing to open source in general. This can help significantly with increasing the number of active contributors in the long term and improving the ecosystem as a whole. |
There was a problem hiding this comment.
Reading more closely into PEP 257, it does not specify that ALL objects (such as classes, functions, methods, etc) should be have docstrings, only the ones which are exported by the module or are public:
All modules should normally have docstrings, and all functions and classes exported by a
module should also have docstrings. Public methods (including the __init__ constructor) should
also have docstrings. A package may be documented in the module docstring of the __init__.py
file in the package directory.
As a result, I think this PR should primarily focus on applying the docstring conventions for all of the exports which are specified in __all__.
_all__ = ["FTP", "error_reply", "error_temp", "error_perm", "error_proto",
"all_errors"]There are sometimes public components of a module that are not included in the __all__ (individual methods in a public class or older modules). The ultimate deciding factor in determining whether something is public or internal is the documentation (the only exception I can think of is the __init__ for a public class). If it's included in the documentation, it is public. In this case, refer to the documentation for ftplib.
In summary, if the function, class, or method is not mentioned in the documentation or included in __all__, it does not need a docstring. Remove all of the changes which add docstrings to internals, and focus on the public ones. This will also significantly simplify the review process for the PR.
|
Normally I would avoid ccing Guido directly, but since he's the only active author of PEP 257, I think his feedback would be especially valuable here. The other author, David Goodger, is not a member of the Python GitHub organization or active on GitHub as far as I can tell (https://github.com/goodger). /cc @gvanrossum |
|
Let's not do this. Conforming to a formatting standard is not enough reason to change existing code that works just fine -- it's perfectly readable as it is. |
I can understand not conforming to the formatting standard, but one thing of important note that I just realized was that That would be much easier to review and provide more of a benefit than just changing the formatting. Edit: For quick reference, it's on line 106. |
|
Sure. You can also reopen this PR and the OP can submit an update -- up to you two. |
Thanks! Especially since this is the author's first PR to any @python repository, I wanted to give them a chance to provide a meaningful contribution. I'll wait on @alanyee to decide what they want to do since it's their PR, either way works me. |
|
I am open to reopening this PR and making the requested changes. |
|
Great! |
Lib/ftplib.py
Outdated
| cmd = 'ACCT ' + password | ||
| return self.voidcmd(cmd) | ||
|
|
||
| def nlst(self, *args): | ||
| '''Return a list of files in a given directory (default the current).''' | ||
| """Return a list of files in a given directory (default the current).""" |
There was a problem hiding this comment.
https://www.python.org/dev/peps/pep-0008/#maximum-line-length
It said For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters.
It looks like a good time to update docstring to follow this limit.
There was a problem hiding this comment.
@alanyee Please follow @aeros167 's feedback.
I think that @aeros167 's feedback is the right way for this moment.
Lib/ftplib.py
Outdated
| hbytes = host.split('.') | ||
| pbytes = [repr(port//256), repr(port%256)] | ||
| bytes = hbytes + pbytes | ||
| cmd = 'PORT ' + ','.join(bytes) | ||
| return self.voidcmd(cmd) | ||
|
|
||
| def sendeprt(self, host, port): | ||
| '''Send an EPRT command with the current host and the given port number.''' | ||
| """Send an EPRT command with the current host and the given port number.""" |
There was a problem hiding this comment.
@alanyee I think you might have misunderstood the changes I had suggested (based on the latest commit):
Could the author potentially open a new PR to just add a docstring for
FTP.__init__(without all of the other formatting changes)?
From my understanding, @gvanrossum had an issue with the changes that were exclusively formatting (such as changing single quotes to double quotes):
Conforming to a formatting standard is not enough reason to change existing code that works just fine -- it's perfectly readable as it is.
My suggestion was to remove every change that was exclusively a formatting change (which was the main focus of the PR initially). The only changes that should remain are ones which add a docstring to a public function, class, or method such as FTP.__init__, where there wasn't one already. Everything else should be removed. This is not everything, but here's a few examples to give you a better idea:
Lib/ftplib.py
Outdated
| @@ -93,8 +92,7 @@ class FTP: | |||
| below for details). | |||
| The download/upload functions first issue appropriate TYPE | |||
| and PORT or PASV commands. | |||
| ''' | |||
|
|
|||
| """ | |||
There was a problem hiding this comment.
Changing the single quotes to double quotes and modifying the spacing are exclusively formatting changes, so this can be removed.
| Initialize host to localhost, port to standard ftp port | ||
| Optional arguments are host (for connect()), | ||
| and user, passwd, acct (for login()) | ||
| """ |
There was a problem hiding this comment.
This change is good, FTP.__init__ is a public method and didn't have a docstring previously.
Lib/ftplib.py
Outdated
| self.passiveserver = val | ||
|
|
||
| # Internal: "sanitize" a string for printing | ||
| # Internal: 'sanitize' a string for printing |
There was a problem hiding this comment.
Here's another example of something that's exclusively a formatting change.
Lib/ftplib.py
Outdated
| - host: hostname to connect to (string, default previous host) | ||
| - port: port to connect to (integer, default previous port) | ||
| - timeout: the timeout to set against the ftp socket(s) | ||
| - source_address: a 2-tuple (host, port) for the socket to bind | ||
| to as its source address before connecting. | ||
| ''' | ||
| """ |
There was a problem hiding this comment.
Here's another example of something that's exclusively a formatting change.
There was a problem hiding this comment.
Thanks for making the recommended changes @alanyee.
From what I can tell, this looks good for the most part. The whitespace changes may need to be removed as well since those could also be considered as being exclusively formatting changes, but we can wait for further core developer feedback on that.
A very minor change I would recommend is adding a period to the end of the docstring sentences. Unlike code comments, docstrings usually have periods. From PEP 257:
The docstring is a phrase ending in a period.
Also, since the focus of the PR has been changed a bit, I would advise adjusting the name to something along the lines of "Add docstring to FTP.__init__". This might help to attract attention from those who are experienced with ftplib in providing feedback for the exact wording of the docstring.
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
|
Sorry, I can't merge this PR. Reason: |
|
Thanks @alanyee for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
I'm having trouble backporting to |
|
Thanks for persevering! This will be backported to 3.8. |
-`"""` over `'''` -no blank line either before or after the docstring. -place the closing quotes on a line by themselves
-`"""` over `'''` -no blank line either before or after the docstring. -place the closing quotes on a line by themselves
-`"""` over `'''` -no blank line either before or after the docstring. -place the closing quotes on a line by themselves
-
"""over'''-no blank line either before or after the docstring.
-place the closing quotes on a line by themselves
Automerge-Triggered-By: @gvanrossum