Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

bpo-44136: removepathlib._Flavour#26141

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

barneygale
Copy link
Contributor

@barneygalebarneygale commentedMay 15, 2021
edited
Loading

Forbpo-24132, we'd like subclasses ofAbstractPath to be able to customize flavour behaviour (likeas_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 ofis_reserved() andmake_uri() (~as_uri()) can be readily moved to intoPurePosixPath andPureWindowsPath, which removes some indirection. This follows the pattern of OS-specific stuff inPosixPath andWindowsPath.

The remaining methods, such assplitroot(), can be pulled intoPure*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 toPurePath 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

@barneygalebarneygaleforce-pushed thebpo-44136-remove-pathlib-flavour branch frome41c4d9 tofc987dcCompareMay 15, 2021 10:58
@barneygalebarneygaleforce-pushed thebpo-44136-remove-pathlib-flavour branch fromfc987dc to4e918e4CompareMay 15, 2021 11:06
@barneygalebarneygale marked this pull request as ready for reviewMay 15, 2021 11:42
@barneygalebarneygale changed the titleWIP: bpo-44136: removepathlib._Flavourbpo-44136: removepathlib._FlavourMay 15, 2021
…` into `PurePath`These are all reasonable default implementations for `AbstractPath`
@barneygalebarneygaleforce-pushed thebpo-44136-remove-pathlib-flavour branch fromb86c025 to6f9fd60CompareMay 15, 2021 18:54
@kfollstad
Copy link
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?

@barneygale
Copy link
ContributorAuthor

barneygale commentedMay 28, 2021
edited
Loading

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". ThePureWindowsPath andWindowsPath classes function exactly as before.

If users want to subclassAbstractPath, but use Windows path syntax (drive letters etc), they can inherit from bothAbstractPath andPureWindowsPath:

classMyWindowsLikePath(pathlib.AbstractPath,pathlib.PureWindowsPath):pass

Does that make sense?

@kfollstad
Copy link
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 aplatform 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 solvingbpo-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.

@barneygale
Copy link
ContributorAuthor

This is a very good point. My main use case here is not so much allowing customization of bothWindowsPath andPosixPath from within a single class, but allowing entirely new kinds ofPath-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!

@barneygalebarneygale marked this pull request as draftMay 28, 2021 12:22
@barneygale
Copy link
ContributorAuthor

The most apparent question for me is: should we mandate explicitTaggedWindowsPath andTaggedPosixPath classes, similar to theWindowsPath andPosixPath classes inpathlib.py? User code would then look like:

classTaggedPath(Path):def__new__(cls,*args,**kwargs):        ...# return either TaggedWindowsPath or TaggedPosixPathdefadd_tag(self,tag):        ...classTaggedWindowsPath(TaggedPath,WindowsPath):passclassTaggedPosixPath(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 explicitTaggedWindowsPath andTaggedPosixPath types is itself too explicit/noisy, we're a bit stuffed becausePosixPath andWindowsPath already work this way.

@barneygale
Copy link
ContributorAuthor

And in fact, I don't think anything in my patch makes the situation better or worse for subclassingPath, but itdoes make things a little easier for subclassingAbstractPath (for embeded/remote filesystems)

@barneygale
Copy link
ContributorAuthor

barneygale commentedMay 28, 2021
edited
Loading

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

classTaggedPath(Path):defadd_tag(self,tag):        ...@TaggedPath.registerclassTaggedWindowsPath(TaggedPath,WindowsPath):pass@TaggedPath.registerclassTaggedPosixPath(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)

@barneygalebarneygale marked this pull request as ready for reviewMay 28, 2021 22:11
@kfollstad
Copy link
Contributor

kfollstad commentedMay 28, 2021
edited
Loading

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

@barneygale
Copy link
ContributorAuthor

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

I still support merging this PR.

@kfollstad
Copy link
Contributor

kfollstad commentedMay 29, 2021
edited by bedevere-bot
Loading

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

@barneygalebarneygale marked this pull request as draftMay 31, 2021 19:37
@barneygale
Copy link
ContributorAuthor

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

@barneygalebarneygalemannequin mentioned this pull requestJun 21, 2022
@projetmbcprojetmbcmannequin mentioned this pull requestJun 21, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@barneygale@kfollstad@the-knights-who-say-ni@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp