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

Appearance settings

GH-127456: pathlib ABCs: remove Parser.sep #127725

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

Closed
wants to merge 3 commits into from

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Dec 7, 2024

Remove Parser.sep, which is the protocol's only non-method member. Instead, we compute the separator from PathBase.__init_subclass__() by calling parser.join('a', 'a').strip('a'), and store the result on the subclass as cls._sep. We also set cls._case_sensitive according to parser.normcase('Aa') == 'Aa', which was previously done in a caching function.

Remove `ParserBase.sep`, which is the class's only non-method member.
Instead, we compute the separator from `PathBase.__init_subclass__()` by
calling `parser.join('a', 'a').strip('a')`, and store the result on the
subclass as `cls._sep`. We also set `cls._case_sensitive` according to
`parser.normcase('Aa') == 'Aa'`, which was previously done in a caching
function.
@encukou
Copy link
Member

encukou commented Dec 9, 2024

I see there's currently no distinction between pathlib-internal attribute names and names that are free for third-party subclasses to use.
Should there be a stronger namespace for these, like _pathlib_sep, _pathlib_globber & _pathlib_stack?
Or should _sep & co. be documented so users know to not override them?

@barneygale
Copy link
Contributor Author

I suppose another option is to make them public as PurePath.separator and PurePath.is_case_sensitive

@barneygale barneygale changed the title GH-127456: pathlib ABCs: remove ParserBase.sep GH-127456: pathlib ABCs: remove Parser.sep Dec 9, 2024
@encukou
Copy link
Member

encukou commented Dec 10, 2024

Hmm. What's the reason for removing sep from Parser, again? That's really the correct place for it.

Should there be a stronger namespace for these, like _pathlib_sep, _pathlib_globber & _pathlib_stack?

I guess that might be for another PR, since it affects _globber and methods as well.

I suppose another option is to make them public as PurePath.separator and PurePath.is_case_sensitive

Right, but then __init_subclass__ should only set them if they're not in __dict__.
It's probably best to keep them as private attributes. Or continue using @functools.cache to avoid magic leaking into the API -- e.g. here, you can't do:

class MyPath(PurePathBase):
    pass
MyPath.parser = MyParser()

@barneygale
Copy link
Contributor Author

All good points. Really my only motivation was to make the Parser interface method-only - partly so that issubclass() checks work, and partly for likely-misguided aesthetic reasons. You've convinced me it isn't a good idea, thank you for your feedback as ever!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.