Also renamed arguments toposixpath.join to matchntpath.join. This is a cosmetic change, but I think they should have matching signatures since they are used interchangeably byos.path andpathlib.

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.

brl0 added2 commits

May 14, 2023 16:25

Rename posixpath.join args to match ntpath.join

6af1da7

Add docstring to ntpath.join

70626f5

bedevere-bot added the awaiting review label

May 14, 2023

This comment was marked as outdated.

Copy link

ghost commentedMay 14, 2023•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.

Merge remote-tracking branch 'upstream/main' into sync_join_funcs

d6bc11b

This comment was marked as duplicate.

barneygale requested changes

May 14, 2023

View reviewed changes

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

barneygaleMay 14, 2023

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

brl0May 14, 2023

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.

Lib/posixpath.py OutdatedShow resolvedHide resolved

bedevere-bot removed the awaiting review label

May 14, 2023

Copy link

bedevere-bot commentedMay 14, 2023

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

bedevere-bot added the awaiting changes label

May 14, 2023

Copy link

Contributor

barneygale commentedMay 14, 2023

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

Fix comment, improve docstring, rename var

5d24b0a

This comment was marked as duplicate.

Check original arg type

28a1c96

This comment was marked as duplicate.

Revert changes to posixpath.join

e2e06f7

This comment was marked as duplicate.

Correct reverted changes

0725beb

This comment was marked as duplicate.

Merge branch 'python:main' into sync_join_funcs

6e0eac4

This comment was marked as duplicate.

📜🤖 Added by blurb_it.

670dc8e

Copy link

Author

brl0 commentedMay 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 onpathlib. I have been contributing to a projectuniversal_pathlib, based onfsspec, and it was in that context I was digging around some of your changes incpython 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 frompathlib, which is why I was looking closely at docstrings and function args.