GeezerVison

Note that a lot of my previous pathlib bugfixes were partly in preparation for making this diff look OK! I was hoping to highlight that Path and _Accessor are conceptually the same at present, and therefore shouldn't be split across two classes. I hope this diff makes that clear! Browsing by commit might be useful also.

I'm pretty sure the test_glob_permissions test wasn't working properly. I had to add a key= argument to sorted() to make things play nice, which makes me think its patching of os.scandir() wasn't visible to pathlib previously.

Bravo! I didn't realize yesterday with #25699 that it was temp code. I had seen your commits and misunderstood. I thought somehow your intent was to add a class in between Path and PurePath which leveraged moving all cwd, resolve, open, touch, into the accessor.

I had done some performance testing that move of cwd, resolve, open, touch into the accessor and noted that it resulted in a significant slow down doing so.

cwd()
python3.9 -m timeit -r 5 -n 100000 -s 'from pathlib import Path' 'Path.cwd()'
100000 loops, best of 5: 4.03 usec per loop
python3.10-with-accessor -m timeit -r 5 -n 100000 -s 'from pathlib import Path' 'Path.cwd()'
100000 loops, best of 5: 206 usec per loop

resolve()
python3.9 -m timeit -r 5 -n 50000 -s 'from pathlib import PosixPath' -s 'p = PosixPath("/usr/bin/python")' 'p.resolve()'
50000 loops, best of 5: 15.4 usec per loop
python3.10-with-accessor -m timeit -r 5 -n 50000 -s 'from pathlib import PosixPath' -s 'p = PosixPath("/usr/bin/python")' 'p.resolve()'
50000 loops, best of 5: 429 usec per loop

open()
python3.9 -m timeit -r 5 -n 100000 -s 'from pathlib import PosixPath' -s 'p = PosixPath("/var/tmp/tmpfile.txt")' 'with open(p, "w"): pass'
100000 loops, best of 5: 14 usec per loop
python3.10-with-accessor -m timeit -r 5 -n 100000 -s 'from pathlib import PosixPath' -s 'p = PosixPath("/var/tmp/tmpfile.txt")' 'with open(p, "w"): pass'
100000 loops, best of 5: 163 usec per loop

touch()
python3.9 -m timeit -r 5 -n 100000 -s 'from pathlib import PosixPath' -s 'p = PosixPath("/var/tmp/tmpfile.txt")' 'p.touch()'
100000 loops, best of 5: 1.89 usec per loop
python3.10-with-accessor -m timeit -r 5 -n 100000 -s 'from pathlib import PosixPath' -s 'p = PosixPath("/var/tmp/tmpfile.txt")' 'p.touch()'
100000 loops, best of 5: 26.1 usec per loop

I have to imagine that moving everything out will do the exact opposite and make all of these slow down go away and speed up all of the other functions that had used the accessor. That said, I'm just a guy out there who happens to be using pathlib, but I think this is brilliant.

Myself, I had been wondering about changing the name of accessor to common (or something similar) having also seen Pitrou's comment to you, since it seemed like a vestigial abstraction. But I like taking it out entirely even better. Remove the cruft and make it faster - this seems like a win!

Yep! And you'll note that the os functions in the resulting code are called from only one Path method. That's to facilitate us adding a new AbstractPath class that sits between PurePath and Path. Certain methods like stat() would be abstract, whereas derived methods like exists() would not.

I'm also planning to perform an optimization pass, as I think there's a few _from_parts() tricks missed here and there that might have slowed things down slightly. Overall I'm expecting the changes to be performance-neutral.

@pitrou I think you wrote this code originally - do you have the time/inclination to review? Thanks so much!

@serhiy-storchaka As you improved the performance of globbing in bpo-26032, do you have any feedback on the Path.scandir() addition? Would you prefer Path._scandir() for example? Thank you.

As for Path.scandir(), if you do not want to extend an API, do not add a public method. And if you want to add a new public method, it is a different issue which should be discussed separately (in particularly it should be shown an advantage over calling os.scandir()).

As for Path.scandir(), if you do not want to extend an API, do not add a public method. And if you want to add a new public method, it is a different issue which should be discussed separately (in particularly it should be shown an advantage over calling os.scandir()).

Thanks very much. I've renamed it to _scandir() as it can stay private for now.

I'll wait for #26153 to land before rebasing.

I've rebased this PR. I believe it's ready for core review!

This PR is stale because it has been open for 30 days with no activity.

I'm still keen to land this if possible. Thanks!

Please avoid force push for future PRs, they make reviewing less pleasant: https://devguide.python.org/pullrequest/

Can you add a news entry? Can be just one line or two about removing an obsolete layer and preparing the terrain for upcoming enhancements.

Can you add a news entry? Can be just one line or two about removing an obsolete layer and preparing the terrain for upcoming enhancements.

I've added a news entry now. I used your wording as it seemed spot on to me!

Thank you for the reviews, everyone. If you can spare the time to continue reviewing this PR series, the next one is at #31085. Thanks so much!