GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-101000: Add os.path.splitroot() by barneygale · Pull Request #101002 · python/cpython

Conversation

@barneygale

@barneygale barneygale commented Jan 12, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@barneygale
pythongh-101000: Add os.path.splitroot()
21c0ba9
@bedevere-bot bedevere-bot added the awaiting review label Jan 12, 2023
@bedevere-bot 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
@barneygale
Use splitroot() from pathlib
836b85d
@barneygale
Use splitroot() from posixpath
bc2d1f9
@barneygale
Use splitroot() from ntpath
ecdc40d
@barneygale
barneygale marked this pull request as ready for review January 12, 2023 22:57
@barneygale
barneygale requested a review from eryksun January 12, 2023 22:57
@barneygale
Optimizations
6592b27
h-vetinari
h-vetinari reviewed Jan 13, 2023
View reviewed changes

@h-vetinari h-vetinari left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread Lib/posixpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/posixpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ntpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ntpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ntpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ntpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
Correct and expand examples in splitroot() docstring
78f4227
@barneygale

barneygale commented Jan 13, 2023

Copy link
Copy Markdown
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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
eryksun
eryksun reviewed Jan 13, 2023
View reviewed changes
Comment thread Lib/ntpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
barneygale and others added 4 commits January 13, 2023 18:50
@barneygale @eryksun
Update Lib/ntpath.py
9726ca4 
Co-authored-by: Eryk Sun <[email protected]>
@barneygale
Use splitroot() from pathlib.PurePath.with_name()
7a6613c
@barneygale
Reduce ntpath.normpath() diff noise
26a8dba
@barneygale
Simplify ntpath.commonpath() now that 'isabs' is unused.
0c237d4
AlexWaygood
AlexWaygood reviewed Jan 15, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale reacted with rocket emoji
All reactions
  • 🚀 1 reaction
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
AlexWaygood self-requested a review January 15, 2023 19:24
barneygale added 4 commits January 15, 2023 19:37
@barneygale
Reduce posixpath.normpath() diff noise
11ed3eb
@barneygale
Improve documentation
2c9eed8
@barneygale
Add whatsnew entry.
8299e96
@barneygale
Simplify ntpath.splitroot() slightly
27ffe37
eryksun
eryksun reviewed Jan 15, 2023
View reviewed changes
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
eryksun
eryksun reviewed Jan 15, 2023
View reviewed changes
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
eryksun
eryksun reviewed Jan 15, 2023
View reviewed changes
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@barneygale @eryksun
Apply suggestions from code review
9beff2a 
Co-authored-by: Eryk Sun <[email protected]>
eryksun
eryksun reviewed Jan 16, 2023
View reviewed changes
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
barneygale and others added 2 commits January 16, 2023 18:43
@barneygale @eryksun
Update Doc/library/os.path.rst
bacdee1 
Co-authored-by: Eryk Sun <[email protected]>
@barneygale
Note that drive may be empty on Windows
4ebe545
AlexWaygood
AlexWaygood reviewed Jan 22, 2023
View reviewed changes
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood
AlexWaygood reviewed Jan 22, 2023
View reviewed changes
Comment thread Doc/library/os.path.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood
AlexWaygood reviewed Jan 22, 2023
View reviewed changes
Comment thread Lib/ntpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood
AlexWaygood reviewed Jan 22, 2023
View reviewed changes
Comment thread Lib/posixpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood
AlexWaygood requested changes Jan 22, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Jan 22, 2023
@bedevere-bot

bedevere-bot commented Jan 22, 2023

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale and others added 2 commits January 22, 2023 22:24
@barneygale @AlexWaygood
Update Doc/library/os.path.rst
3663237 
Co-authored-by: Alex Waygood <[email protected]>
@barneygale
Add tests for bytes (POSIX only) and path-like objects (both platforms)
053729d
AlexWaygood
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 Resolved

Uh oh!

There was an error while loading. Please reload this page.
barneygale added 2 commits January 22, 2023 22:53
@barneygale
Add tests for mixed path separators (Windows only)
694f093
@barneygale
Remove errant newline.
e99e3cd
AlexWaygood
AlexWaygood reviewed Jan 22, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@barneygale

barneygale commented Jan 22, 2023

Copy link
Copy Markdown
Contributor Author

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented Jan 22, 2023

Copy link
Copy Markdown
Member

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale added 2 commits January 22, 2023 23:43
@barneygale
Move most test cases from test_splitdrive to test_splitroot
f618a00
@barneygale
Mention pathlib performance improvement in news entry.
1c522c9
AlexWaygood
AlexWaygood approved these changes Jan 22, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

@eryksun, Any further comments from you? :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Jan 22, 2023
@AlexWaygood
Merge branch 'main' into pythongh-101000-splitroot
df17269
@AlexWaygood AlexWaygood added type-feature A feature request or enhancement topic-pathlib labels Jan 23, 2023
@AlexWaygood

AlexWaygood commented Jan 24, 2023

Copy link
Copy Markdown
Member

(Planning to merge in a few days, unless @eryksun has any further feedback :)
barneygale reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
AlexWaygood merged commit e5b08dd into python:main Jan 27, 2023
@bedevere-bot 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
@barneygale @eryksun
@AlexWaygood @mdboom
pythongh-101000: Add os.path.splitroot() (python#101002)
a20b37a 
Co-authored-by: Eryk Sun <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
@barneygale barneygale mentioned this pull request Apr 8, 2023
Vestigial pathlib 'flavour' tests make assertions about internal method #103379
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Aug 6, 2026
bench, parity: widen six floored-denominator ratio gates; scope the platform-specific parity fixtures pyre-lang/pyre#1071
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eryksun eryksun eryksun left review comments
@AlexWaygood AlexWaygood AlexWaygood approved these changes
+1 more reviewer
@h-vetinari h-vetinari h-vetinari left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-pathlib type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@barneygale @bedevere-bot @AlexWaygood @eryksun @h-vetinari
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.