Skip to content
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

gh-101000: Add os.path.splitroot() #101002

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

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Jan 12, 2023

This PR introduces os.path.splitroot(). See #101000 for motivation.

In ntpath, the implementation derives from splitdrive(). The splitdrive() function now calls splitroot(), and returns drive, root + tail. Other functions now call splitroot() rather than splitdrive(). In most cases this replaces their own parsing of path roots. It also avoids adding a stack frame.

In posixpath, the normpath() function now calls splitroot() rather than parsing path roots itself.

In pathlib, path constructors now call splitroot() rather than using a slow OS-agnostic implementation. Performance:

$ ./python -m timeit -s 'from pathlib import PureWindowsPath' 'PureWindowsPath("C:/", "foo", "bar")'
50000 loops, best of 5: 6.04 usec per loop  # before
50000 loops, best of 5: 4.03 usec per loop  # after
$ ./python -m timeit -s 'from pathlib import PurePosixPath' 'PurePosixPath("/", "etc", "hosts")'
100000 loops, best of 5: 3.11 usec per loop  # before
100000 loops, best of 5: 2.77 usec per loop  # after

Future work:

  • Improve performance by using native nt._path_splitroot()

@barneygale barneygale marked this pull request as ready for review Jan 12, 2023
@barneygale barneygale requested a review from eryksun Jan 12, 2023
Copy link

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Drive-by comments from seeing this PR on discourse, feel free to disregard if I'm saying/asking something stupid.

Lib/posixpath.py Outdated Show resolved Hide resolved
Lib/posixpath.py Show resolved Hide resolved
Lib/ntpath.py Show resolved Hide resolved
Lib/ntpath.py Outdated Show resolved Hide resolved
Lib/ntpath.py Show resolved Hide resolved
Lib/ntpath.py Outdated Show resolved Hide resolved
@barneygale
Copy link
Contributor Author

Drive-by comments from seeing this PR on discourse, feel free to disregard if I'm saying/asking something stupid.

Thanks for the review! All good feedback I think!

Lib/ntpath.py Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I'm persuaded that this is a good idea. Thanks for working on this!

Here's a docs review. Haven't got to looking at the implementation yet (will do soon).

Doc/library/os.path.rst Outdated Show resolved Hide resolved
Doc/library/os.path.rst Outdated Show resolved Hide resolved
Doc/library/os.path.rst Outdated Show resolved Hide resolved
Doc/library/os.path.rst Outdated Show resolved Hide resolved
Doc/library/os.path.rst Outdated Show resolved Hide resolved
Doc/library/os.path.rst Show resolved Hide resolved
@AlexWaygood AlexWaygood self-requested a review Jan 15, 2023
Doc/library/os.path.rst Outdated Show resolved Hide resolved
Doc/library/os.path.rst Outdated Show resolved Hide resolved
Doc/library/os.path.rst Outdated Show resolved Hide resolved
Co-authored-by: Eryk Sun <eryksun@gmail.com>
Doc/library/os.path.rst Outdated Show resolved Hide resolved
barneygale and others added 2 commits Jan 16, 2023
>>> splitroot('/etc/hosts')
('', '/', 'etc/hosts')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
>>> splitroot('/etc/hosts')
('', '/', 'etc/hosts')
>>> splitroot('home/barney')
('', '', 'home/barney')
>>> splitroot('/home/barney')
('', '/', 'home/barney')
>>> splitroot('//home/barney')
('', '//', 'home/barney')
>>> splitroot('///home/barney')
('', '/', '//home/barney')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps getting too long IMHO. @AlexWaygood what do you think? (and below)

Copy link
Contributor

Choose a reason for hiding this comment

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

The example with a relative path could be skipped as something most people will understand intuitively. I think it's helpful to show "/" vs "//" vs "///", or at least "//" vs "///". I think the result with "//" may be surprising because Linux and macOS handle it the same as "/" and "///".

>>> splitroot('Windows/notepad')
('', '', 'Windows/notepad')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
>>> splitroot('Windows/notepad')
('', '', 'Windows/notepad')
>>> splitroot('Users/Barney')
('', '', 'Users/Barney')
>>> splitroot('/Users/Barney')
('', '/', 'Users/Barney')

Doc/library/os.path.rst Outdated Show resolved Hide resolved
Co-authored-by: Eryk Sun <eryksun@gmail.com>
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.

None yet

5 participants