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

Teach zipp.Path some additional pathlib.Path methods. #85

Merged
merged 11 commits into from Nov 25, 2022

Conversation

Julian
Copy link
Contributor

@Julian Julian commented Nov 2, 2022

Covers:

* Path.match
* Path.glob
* Path.rglob
* Path.relative_to
* Path.is_symlink (with trivial implementation given the lack of
  symlink support currently)

along with an implementation of Path.eq for testing/comparison purposes.

Part of the implementation here makes use of CPython internals (from the pathlib module) but it seems to handle fixing that requires some refactoring upstream.

Covers:

    * Path.match
    * Path.glob
    * Path.rglob
    * Path.relative_to
    * Path.is_symlink (with trivial implementation given the lack of
      symlink support currently)

along with an implementation of Path.__eq__ for testing/comparison
purposes.

Part of the implementation here makes use of CPython internals (from
the pathlib module) but it seems to handle fixing that requires some
refactoring upstream.
zipp/__init__.py Show resolved Hide resolved
Copy link
Owner

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Overall this looks good. I'm a little worried about the introduction of platform-specific concerns and especially about the correctness of relative_to, but most of my other concerns are nitpicks or things to consider for subsequent iteration.

zipp/__init__.py Outdated Show resolved Hide resolved
zipp/__init__.py Outdated Show resolved Hide resolved
zipp/__init__.py Outdated Show resolved Hide resolved
zipp/__init__.py Outdated Show resolved Hide resolved
zipp/__init__.py Outdated Show resolved Hide resolved
zipp/__init__.py Outdated Show resolved Hide resolved
zipp/__init__.py Show resolved Hide resolved
zipp/__init__.py Outdated Show resolved Hide resolved
@jaraco
Copy link
Owner

jaraco commented Nov 25, 2022

Tests are failing on Windows, I suspect because relative_to is failing to match the descendants.

@jaraco
Copy link
Owner

jaraco commented Nov 25, 2022

Tests are failing on Windows, I suspect because relative_to is failing to match the descendants.

The issue is that str() of a Path forces separators to os.sep.

(Pdb) cand.relative_to(self)
WindowsPath('b/c.txt')
(Pdb) str(cand.relative_to(self))
'b\\c.txt'

@jaraco jaraco merged commit 2c66630 into jaraco:main Nov 25, 2022
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.

None yet

2 participants