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

MAINT: Cleanup compatibility code for pathlib #17529

Merged
merged 1 commit into from
Oct 10, 2020

Conversation

eric-wieser
Copy link
Member

Path can now never be none, and PurePath is not used and not in __all__.

`Path` can now never be none, and `PurePath` is not used and not in `__all__`.
@BvB93
Copy link
Member

BvB93 commented Oct 10, 2020

This may be beyond the scope of this PR, but do we even need the is_pathlib_path function?

The only place where it seems to be used is in memmap.__new__ and replacing it with an
unconditional call to either os.fspath or os.fsdecode will also take care of any pathlib.Path instances.

@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 10, 2020

No we don't - but we probably can't remove things from this module without deprecating them first.

We should certainly change all the callers of these useless aliases.

@BvB93
Copy link
Member

BvB93 commented Oct 10, 2020

No we don't - but we probably can't remove things from this module without deprecating them first.

Isn't compat a private module though?

@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 10, 2020

I don't think we ever made it clear what is and is not public - it doesn't have any underscores in it, so the default user may assume it public, even if that was never our intent. It's a thorny enough issue to definitely be out of scope for this PR.

The C compat header has similar issues.

@BvB93
Copy link
Member

BvB93 commented Oct 10, 2020

Fair enough, let's keep this PR as it is then.

@charris charris merged commit 2866c83 into numpy:master Oct 10, 2020
@charris
Copy link
Member

charris commented Oct 10, 2020

Thanks Eric.

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.

None yet

3 participants