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 — bpo-44136: remove `pathlib._Flavour` by barneygale · Pull Request #26141 · python/cpython

Conversation

@barneygale

@barneygale barneygale commented May 15, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

For bpo-24132, we'd like subclasses of AbstractPath to be able to customize flavour behaviour (like as_uri()) without needing to promote _Flavour to a public class.

(Copy-pasted bug description:)

Following #18841, #25264 and #25701, pathlib._Flavour and its subclasses are looking a bit pointless.

The implementations of is_reserved() and make_uri() (~as_uri()) can be readily moved to into PurePosixPath and PureWindowsPath, which removes some indirection. This follows the pattern of OS-specific stuff in PosixPath and WindowsPath.

The remaining methods, such as splitroot(), can be pulled into Pure*Path with an underscore prefix.

I'm generally a believer in composition over inheritance, but in this case _Flavour seems too small and too similar to PurePath to separate out into 3 extra classes.

There should be no impact on public APIs or performance.

I've tried to split the changes up across commits; it may be easier to browse the diff that way.

https://bugs.python.org/issue44136

Sorry, something went wrong.

Uh oh!

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

All reactions
barneygale added 4 commits May 14, 2021 20:19
@barneygale
Remove flavour.is_reserved()
3655d5e
@barneygale
Remove flavour.make_uri()
0faa6d0
@barneygale
Remove flavour.join(), flavour.sep and flavour.altsep
3094398
@barneygale
Remove flavour.join_parsed_parts()
00f6126
@bedevere-bot bedevere-bot added the awaiting review label May 15, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 15, 2021
barneygale added 5 commits May 15, 2021 09:45
@barneygale
Remove flavour.parse_parts(), flavour.splitroot() and `flavour.pa…
6ea7eae
…thmod`
@barneygale
Remove flavour.compile_pattern()
66f5453
@barneygale
Remove flavour.is_supported
3d27707
@barneygale
Remove flavour.has_drv.
b4beecc
@barneygale
Remove flavour.casefold() and flavour.casefold_parts()
3cf15c6
@barneygale
barneygale force-pushed the bpo-44136-remove-pathlib-flavour branch from e41c4d9 to fc987dc Compare May 15, 2021 10:58
barneygale added 2 commits May 15, 2021 11:59
@barneygale
Remove _Flavour, _WindowsFlavour and _PosixFlavour
0897d9d
@barneygale
Simplify Path construction.
4e918e4
@barneygale
barneygale force-pushed the bpo-44136-remove-pathlib-flavour branch from fc987dc to 4e918e4 Compare May 15, 2021 11:06
barneygale added 2 commits May 15, 2021 12:10
@barneygale
Move Windows constants to module scope.
f0beb66
@barneygale
Remove Path._supported
22942e4
@barneygale
barneygale marked this pull request as ready for review May 15, 2021 11:42
@barneygale barneygale changed the title WIP: bpo-44136: remove pathlib._Flavour bpo-44136: remove pathlib._Flavour May 15, 2021
@barneygale
Move PurePosixPath._splitroot(), is_absolute() and `is_reserved()…
6f9fd60
…` into `PurePath`

These are all reasonable default implementations for `AbstractPath`
@barneygale
barneygale force-pushed the bpo-44136-remove-pathlib-flavour branch from b86c025 to 6f9fd60 Compare May 15, 2021 18:54
@kfollstad

kfollstad commented May 27, 2021

Copy link
Copy Markdown
Contributor

@barneygale I thought your design was Path which inherits from AbstractPath which inherits from PurePath?

If that is the case, then what I don't understand about this is if you move all of the Windows flavour logic into PureWindowsPath, then how can someone who wants to inherit from AbstractPath do anything with Windows if they want to? There will be no Windows flavour and there will be no PureWindowsPath in the mro. I am misunderstanding something?

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale

barneygale commented May 28, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I thought your design was Path which inherits from AbstractPath which inherits from PurePath?

This is correct!

how can someone who wants to inherit from AbstractPath do anything with Windows if they want to?

It depends what you mean by "do anything with Windows". The PureWindowsPath and WindowsPath classes function exactly as before.

If users want to subclass AbstractPath, but use Windows path syntax (drive letters etc), they can inherit from both AbstractPath and PureWindowsPath:

class MyWindowsLikePath(pathlib.AbstractPath, pathlib.PureWindowsPath):
    pass

Does that make sense?

All reactions

Sorry, something went wrong.

Uh oh!

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

@kfollstad

kfollstad commented May 28, 2021

Copy link
Copy Markdown
Contributor

No, sorry I definitely was not clear enough.

What I am saying is that you seem to be assuming the primacy of Posix. For example, how would I write a platform agnostic version of the following with your AbstractPath?

class TaggedPath(AbstractPath):
    def add_tag(self, tag):
        ...

or would it be:

class TaggedPath(AbstractPath, WindowsPurePath):
    def add_tag(self, tag):
        ...

Does it make sense what I am trying to point out?

The reason that Path isn't subclassable is that it is a factory which is dependent on its two subclasses, and upon instantiation a decision has to be made about what class to return, PosixPath or WindowsPath.

If I am understanding correctly it seems like your solution with removing flavour from AbstractPath ignores that Windows-syntax paths exist. If you ignore that Windows-syntax paths exist, then of course there is no decision to be made.

As such, to me this doesn't really seem to working towards solving bpo-24132 because if the user in the original post was just assuming a specific platform, they could have subclassed either PosixPath or WindowsPath and had no problems. The trick is to make it work while not assuming a specific platform. Maybe I am missing something, but I don't understand how that will be possible if you strip out the Windows-style path syntax into PureWindowsPath like you did in these commits.

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale

barneygale commented May 28, 2021

Copy link
Copy Markdown
Contributor Author

This is a very good point. My main use case here is not so much allowing customization of both WindowsPath and PosixPath from within a single class, but allowing entirely new kinds of Path-like paths to exist that might be backed by zip files, FTP servers, etc.

I'll consider this carefully. Making this MR draft for now!

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale
barneygale marked this pull request as draft May 28, 2021 12:22
@barneygale

barneygale commented May 28, 2021

Copy link
Copy Markdown
Contributor Author

The most apparent question for me is: should we mandate explicit TaggedWindowsPath and TaggedPosixPath classes, similar to the WindowsPath and PosixPath classes in pathlib.py? User code would then look like:

class TaggedPath(Path):
    def __new__(cls, *args, **kwargs):
        ...  # return either TaggedWindowsPath or TaggedPosixPath

    def add_tag(self, tag):
        ...

class TaggedWindowsPath(TaggedPath, WindowsPath):
    pass

class TaggedPosixPath(TaggedPath, PosixPath):
    pass

If defining __new__ to return the right thing feels like boilerplate, we could add some sort of registration in __subclasshook__.

If having explicit TaggedWindowsPath and TaggedPosixPath types is itself too explicit/noisy, we're a bit stuffed because PosixPath and WindowsPath already work this way.

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale

barneygale commented May 28, 2021

Copy link
Copy Markdown
Contributor Author

And in fact, I don't think anything in my patch makes the situation better or worse for subclassing Path, but it does make things a little easier for subclassing AbstractPath (for embeded/remote filesystems)

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale

barneygale commented May 28, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

A bit more mucking around in the code. It's very possible to generalise Path.__new__, so I think we can support:

class TaggedPath(Path):
    def add_tag(self, tag):
        ...

@TaggedPath.register
class TaggedWindowsPath(TaggedPath, WindowsPath):
    pass

@TaggedPath.register
class TaggedPosixPath(TaggedPath, PosixPath):
    pass

i.e. no need to require users to write a __new__ function.

This PR is actually helps a little with the work, because we can have some notion of "not supported" on Path (+ subclasses) that's independent of flavour (which Path lacks entirely atm)

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale
barneygale marked this pull request as ready for review May 28, 2021 22:11
@kfollstad

kfollstad commented May 28, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

If having explicit TaggedWindowsPath and TaggedPosixPath types is itself too explicit/noisy, we're a bit stuffed because PosixPath and WindowsPath already work this way.

Yes, I agree. That Path/PosixPath/WindowsPath work that way is the origin of all of these complications.
I have also been challenged by this lack of subclassing and have been thinking about it and working on a PR for some time.

A while ago I recognized that the crux of this problems is the inverted dependency relationship that Path has with PosixPath and WindowsPath. Which by the way is why up until recently I viewed what you and I were working on as orthogonal - because you were reorganizing the internal components to create a new abstraction for Zip files, etc. However my issue was with seeing if reorganizing the factory structure was a workable idea. However, I think, unless I am mistaken, that everything that you are trying to accomplish with your AbstractPath also falls out of the new classes I defined.

However, that said it seems like there is still all sorts of issues that you have identified in internals that need work on. You definitely understand the nitty gritty of those far better than I do. Hopefully between the two of us we can work to make Pathlib a little more usable.

barneygale reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@barneygale

barneygale commented May 29, 2021

Copy link
Copy Markdown
Contributor Author

Further discussion here: https://discuss.python.org/t/fixing-subclassing-in-pathlib/8983

I still support merging this PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

@kfollstad

kfollstad commented May 29, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

And in fact, I don't think anything in my patch makes the situation better or worse for subclassing Path, but it does make things a little easier for sub classing AbstractPath (for embeded/remote filesystems)

Perhaps, but I think there are other ways of accomplishing what you are trying to do, without taking out flavour with these commits. In particular, by adding flavour (_generic_flavor = _PosixFlavour()) to your class at the time it is instantiated.

In this way, your AbstractPath could be implemented without having to rip out flavour. Path.__new__ will override this, preventing any problems with inheriting from AbstractPath.

If that is the case, the whole issue of how to solve subclassing in bpo-24132, that I am trying to solve with #26438 can be settled later, after all possibilities are considered. Perhaps there is a better way than what I proposed.

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale
barneygale marked this pull request as draft May 31, 2021 19:37
@barneygale

barneygale commented Jun 13, 2021

Copy link
Copy Markdown
Contributor Author

Withdrawing this PR as I'm going to take a more piecemeal approach. Hopeful we can get rid of flavours without adding anything to PurePath + friends.

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale barneygale closed this Jun 13, 2021
@barneygale barneygale mannequin mentioned this pull request Jun 21, 2022
Remove pathlib flavours #88302
Closed
@projetmbc projetmbc mannequin mentioned this pull request Jun 21, 2022
Direct sub-classing of pathlib.Path #68320
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

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.

4 participants

@barneygale @kfollstad @the-knights-who-say-ni @bedevere-bot
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.