-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
Add docstring for join
in ntpath.py
#18555
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
Conversation
Adapted from the corresponding docstring in posixpath.py
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this 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 the contribution, we look forward to reviewing it! |
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 the PR and welcome @edschofield!
Note that the CLA will need to be signed and processed (typically takes ~24-48 hours after signing) before the PR can be merged.
Here's a few suggestions in the meantime:
"""Join two or more pathname components, inserting '\\' as needed. | ||
If any component is an absolute path, all previous path components | ||
will be discarded. An empty last part will result in a path that | ||
ends with a separator.""" |
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.
Formatting:
Docstrings in the standard library (particularly more recently added ones) should comply with the formatting standards of PEP 257. In order to make this one compliant, it should contain one line of whitespace in between the first summary sentence and the sentences after it. Also, the final set of triple quotes """
should be on a line by itself (when the docstring is more than one line). See the PEP for more details.
Wording/Details:
-
"Join two or more pathname components" is not quite correct. Although it's usually used for this purpose, it can also be used on a single path; meaning "one or more" would be more correct in this case.
-
The separator character is inserted in between every path that is not empty; IMO this should be specified instead of just stating "inserting '\\' as needed", which is a bit ambiguous.
-
There is a Windows-specific paragraph of the documentation for
os.path.join()
, which I think should be included (at least partially) in the docstring here, since this section is specifically for the Windows implementation. For reference, here's the paragraph:
On Windows, the drive letter is not reset when an absolute path component (e.g., r'\foo') is encountered. If a
component contains a drive letter, all previous components are thrown away and the drive letter is reset.
Note that since there is a current directory for each drive, os.path.join("c:", "foo") represents a path relative to
the current directory on drive C: (c:foo), not c:\foo.
For conciseness, I think the last sentence can be excluded, but the rest should probably be in the docstring. I'll admit that part is a bit subjective though.
Since this PR is simply adding a docstring, a bpo issue and news entry is not needed. |
@edschofield, please sign the CLA as previously requested and please also address the code review. Thank you! |
This PR is stale because it has been open for 30 days with no activity. If the CLA is not signed within 14 days, it will be closed. See also https://devguide.python.org/pullrequest/#licensing |
Closing this stale PR because the CLA is still not signed. |
Adapted from the corresponding docstring in posixpath.py