Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-126705: Make os.PathLike more like a protocol #126706

Merged
merged 3 commits into from
Nov 12, 2024
Merged

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Nov 11, 2024

This MR makes os.PathLike more protocol-like in two ways:

  1. adds it to typing._PROTO_ALLOWLIST so it can be used as a base class in other protocols
  2. use class PathLike(metaclass=abc.ABCMeta) instead of class PathLike(abc.ABC). This aligns with the other protocol-like ABCs in collections.abc. Actual protocols do have that metaclass but don't have the abc.ABC base class. Doing this makes it a little smoother for typeshed to define the class as class PathLike(Protocol) in the stubs.

Copy link
Contributor

@bswck bswck left a comment

Choose a reason for hiding this comment

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

On-topic: LGTM
Off-topic:
Makes me wonder why not allow ABC in bases here:

base in {object, Generic}

It's such a common base class, is there anything mutually exclusive with abstract classes and protocols? This PR proves false.

@tungol
Copy link
Contributor Author

tungol commented Nov 11, 2024

I think the need for the explicit allowlist is that not all ABCs are protocol-like; only ABCs with a __subclass_hook__ that's properly constructed for that purpose should be mixed with protocols. It's not that they're mutually exclusive, it's just that most of them aren't suitable.

@tungol
Copy link
Contributor Author

tungol commented Nov 11, 2024

I think I see the confusion: being able to use PathLike as a protocol base only requires adding it to _PROTO_ALLOWLIST. Adjusting the inheritance of the class is not required for that functionality, it just makes it a little cleaner. Of my two points, number 1 can be implemented without number 2 (or vice versa).

Lib/os.py Outdated Show resolved Hide resolved
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@AlexWaygood AlexWaygood enabled auto-merge (squash) November 12, 2024 17:46
@AlexWaygood AlexWaygood merged commit a83472f into python:main Nov 12, 2024
40 checks passed
@tungol tungol deleted the pathlike branch November 12, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants