Skip to content

Match docstring and args for posixpath.join and ntpath.join #104478

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

brl0
Copy link

@brl0 brl0 commented May 14, 2023

Adpated docstring from posixpath.join to ntpath.join, which was previously blank.

Also renamed arguments to posixpath.join to match ntpath.join. This is a cosmetic change, but I think they should have matching signatures since they are used interchangeably by os.path and pathlib.

Here are the current signatures:

function signature
posixpath.join def join(a, *p)
ntpath.join def join(path, *paths)

This change is not much more than fixing a typo, so I didn't open an issue for this, but would be willing to do so if that is preferred.
Also, this change is probably not news-worthy, but I can add an entry if needed.

@ghost
Copy link

ghost commented May 14, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

Lib/ntpath.py Outdated
Comment on lines 109 to 110
If any component is an absolute path, all previous path components
will be discarded. An empty last part will result in a path that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ntpath, "absolute path" means a path with a root, such as "\bar". If this path does not have a drive letter, then the previous drive letter (if any) is retained. So it's not right to say that all previous components will be discarded -- it's a little more subtle than that!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification! I made a change to be more descriptive, let me know if it still needs work.

@barneygale
Copy link
Contributor

Adding a docstring for os.path.join() on Windows is worthy of an issue and a news entry, IMO.

@brl0
Copy link
Author

brl0 commented May 15, 2023

@barneygale,

Thanks for your quick review and feedback, it is much appreciated. I have made the requested changes; please review again.

Also, thanks for your great work on pathlib. I have been contributing to a project universal_pathlib, based on fsspec, and it was in that context I was digging around some of your changes in cpython to begin with. I was trying to figure out what would need to be adapted to properly implement a new path flavour module for classes inheriting from pathlib, which is why I was looking closely at docstrings and function args.

@arhadthedev
Copy link
Member

Do we need a news entry for documentation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants