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

barneygale

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()

Issue: Add os.path.splitroot() function #101000

h-vetinari

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

barneygale

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!

AlexWaygood

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

AlexWaygood

I believe there are currently no tests that os.path.splitroot works with os.PathLike objects. Just trivial tests like this should do fine, but we should make sure it's tested:

cpython/Lib/test/test_ntpath.py

Lines 946 to 947 in 0e75a55

    
           def test_path_splitdrive(self): 
        
               self._check_function(self.path.splitdrive)

bedevere-bot

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 phrase I 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.

AlexWaygood

This is looking really good to me now, and I'm very close to hitting "approve". My only concern (other than my comment about the NEWS entry) is that I'm still not sure the test coverage is quite there. It looks like the tests for splitdrive() account for a lot of edge cases that aren't really tackled in the tests for splitroot() yet, e.g.

cpython/Lib/test/test_ntpath.py

Lines 117 to 119 in e99e3cd

    
           # Issue #19911: UNC part containing U+0130 
        
           self.assertEqual(ntpath.splitdrive('//conky/MOUNTPOİNT/foo/bar'), 
        
                            ('//conky/MOUNTPOİNT', '/foo/bar'))

and

cpython/Lib/test/test_ntpath.py

Lines 129 to 130 in e99e3cd

    
           tester('ntpath.splitdrive("//?/VOLUME{00000000-0000-0000-0000-000000000000}/spam")', 
        
                  ('//?/VOLUME{00000000-0000-0000-0000-000000000000}', '/spam'))

It's true that, since splitdrive() now uses splitroot(), these edge cases are in some sense already covered -- the tests for splitdrive() will start failing if a bug is introduced to splitroot() at some later date in the future. But it will be highly confusing if the tests for splitdrive() start failing, yet the tests for splitroot() all still pass, when the bug is actually in the implementation for splitroot().

barneygale

Hm. I could rename test_splitdrive to test_splitroot and adjust all the test cases - would that address your concern? (I'd add a new set of tests for splitdrive() that would cover just the basics)

AlexWaygood

Hm. I could rename test_splitdrive to test_splitroot and adjust all the test cases - would that address your concern? (I'd add a new set of tests for splitdrive() that would cover just the basics)

Yeah, I think that would make sense!

AlexWaygood

Looks great to me. Thanks, as ever, for your patience and perseverance!

@eryksun, Any further comments from you? :)

AlexWaygood

(Planning to merge in a few days, unless @eryksun has any further feedback :)

pythongh-101000 : Add os.path.splitroot()

21c0ba9

bedevere-bot added the awaiting review label Jan 12, 2023

bedevere-bot mentioned this pull request Jan 12, 2023

Add os.path.splitroot() function #101000

Closed

barneygale added 3 commits January 12, 2023 21:52

Use splitroot() from pathlib

836b85d

Use splitroot() from posixpath

bc2d1f9

Use splitroot() from ntpath

ecdc40d

barneygale marked this pull request as ready for review January 12, 2023 22:57

barneygale requested a review from eryksun January 12, 2023 22:57

Optimizations

6592b27

h-vetinari reviewed Jan 13, 2023

View reviewed changes

Correct and expand examples in splitroot() docstring

78f4227

eryksun reviewed Jan 13, 2023

View reviewed changes

Comment thread Lib/ntpath.py Outdated Show resolved Hide resolved

barneygale and others added 4 commits January 13, 2023 18:50

Update Lib/ntpath.py …

9726ca4

Co-authored-by: Eryk Sun <eryksun@gmail.com>

Use splitroot() from pathlib.PurePath.with_name()

7a6613c

Reduce ntpath.normpath() diff noise

26a8dba

Simplify ntpath.commonpath() now that 'isabs' is unused.

0c237d4

AlexWaygood reviewed Jan 15, 2023

View reviewed changes

AlexWaygood self-requested a review January 15, 2023 19:24

barneygale added 4 commits January 15, 2023 19:37

Reduce posixpath.normpath() diff noise

11ed3eb

Improve documentation

2c9eed8

Add whatsnew entry.

8299e96

Simplify ntpath.splitroot() slightly

27ffe37

eryksun reviewed Jan 15, 2023

View reviewed changes

Comment thread Doc/library/os.path.rst Outdated Show resolved Hide resolved

eryksun reviewed Jan 15, 2023

View reviewed changes

Comment thread Doc/library/os.path.rst Outdated Show resolved Hide resolved

eryksun reviewed Jan 15, 2023

View reviewed changes

Comment thread Doc/library/os.path.rst Outdated Show resolved Hide resolved

Apply suggestions from code review …

9beff2a

Co-authored-by: Eryk Sun <eryksun@gmail.com>

eryksun reviewed Jan 16, 2023

View reviewed changes

Comment thread Doc/library/os.path.rst Outdated Show resolved Hide resolved

barneygale and others added 2 commits January 16, 2023 18:43

Update Doc/library/os.path.rst …

bacdee1

Co-authored-by: Eryk Sun <eryksun@gmail.com>

Note that drive may be empty on Windows

4ebe545

Adjust first paragraph to use prose …

0e75a55

... and not belabour the fact that the empty string may be returned as any/all items.

AlexWaygood reviewed Jan 22, 2023

View reviewed changes

Comment thread Doc/library/os.path.rst Show resolved Hide resolved

AlexWaygood reviewed Jan 22, 2023

View reviewed changes

Comment thread Doc/library/os.path.rst Outdated Show resolved Hide resolved

AlexWaygood reviewed Jan 22, 2023

View reviewed changes

Comment thread Lib/ntpath.py Show resolved Hide resolved

AlexWaygood reviewed Jan 22, 2023

View reviewed changes

Comment thread Lib/posixpath.py Show resolved Hide resolved

AlexWaygood requested changes Jan 22, 2023

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 22, 2023

barneygale and others added 2 commits January 22, 2023 22:24

Update Doc/library/os.path.rst …

3663237

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Add tests for bytes (POSIX only) and path-like objects (both platforms)

053729d

AlexWaygood reviewed Jan 22, 2023

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2023-01-12-21-22-20.gh-issue-101000.wz4Xgc.rst Outdated Show resolved Hide resolved

barneygale added 2 commits January 22, 2023 22:53

Add tests for mixed path separators (Windows only)

694f093

Remove errant newline.

e99e3cd

AlexWaygood reviewed Jan 22, 2023

View reviewed changes

barneygale added 2 commits January 22, 2023 23:43

Move most test cases from test_splitdrive to test_splitroot

f618a00

Mention pathlib performance improvement in news entry.

1c522c9

AlexWaygood approved these changes Jan 22, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jan 22, 2023

Merge branch 'main' into pythongh-101000 -splitroot

df17269

AlexWaygood added type-feature A feature request or enhancement topic-pathlib labels Jan 23, 2023

AlexWaygood merged commit e5b08dd into python:main Jan 27, 2023

bedevere-bot removed the awaiting merge label Jan 27, 2023

mdboom pushed a commit to mdboom/cpython that referenced this pull request Jan 31, 2023

pythongh-101000 : Add os.path.splitroot() (python#101002 ) …

a20b37a

Co-authored-by: Eryk Sun <eryksun@gmail.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

barneygale mentioned this pull request Apr 8, 2023

Vestigial pathlib 'flavour' tests make assertions about internal method #103379

Closed

	def test_path_splitdrive(self):
	self._check_function(self.path.splitdrive)

	# Issue #19911: UNC part containing U+0130
	self.assertEqual(ntpath.splitdrive('//conky/MOUNTPOİNT/foo/bar'),
	('//conky/MOUNTPOİNT', '/foo/bar'))

	tester('ntpath.splitdrive("//?/VOLUME{00000000-0000-0000-0000-000000000000}/spam")',
	('//?/VOLUME{00000000-0000-0000-0000-000000000000}', '/spam'))

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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