Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

edschofield
Copy link

Adapted from the corresponding docstring in posixpath.py

Adapted from the corresponding docstring in posixpath.py
@the-knights-who-say-ni
Copy link

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 username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@edschofield

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!

Copy link
Contributor

@aeros aeros left a 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:

Comment on lines +78 to +81
"""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."""
Copy link
Contributor

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:

  1. "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.

  2. 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.

  3. 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.

@aeros
Copy link
Contributor

aeros commented Feb 19, 2020

Since this PR is simply adding a docstring, a bpo issue and news entry is not needed.

@csabella
Copy link
Contributor

@edschofield, please sign the CLA as previously requested and please also address the code review. Thank you!

@github-actions
Copy link

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

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 19, 2022
@github-actions
Copy link

github-actions bot commented Mar 6, 2022

Closing this stale PR because the CLA is still not signed.

@github-actions github-actions bot closed this Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review docs Documentation in the Doc dir skip issue skip news stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants