cloudpathlib icon indicating copy to clipboard operation
cloudpathlib copied to clipboard

Anypath abstractmethods

Open chbehrens opened this issue 2 years ago β€’ 4 comments

Disclaimer: I quickly hacked this as a draft/suggestion. There might be better/nicer methods to achieve the same.

I suggest to add some abstractmethods to AnyPath so that the methods available for both CloudPath and pathlib.Path can be seen when AnyPath is used as type annotation. It would be nice if there are more elegant solutions but I think the overall behavior would be useful.

Currently missing:

  • Docstrings for functions that differ between pathlib.Path and CloudPath
  • CloudPathMeta could inherit from AnyPathMeta to avoid code duplication

I probably won't be around the next days but wanted to leave this as starting point to discuss.

chbehrens avatar Aug 19 '22 18:08 chbehrens

Codecov Report

Merging #266 (3d455b3) into master (98ed2e5) will decrease coverage by 2.4%. The diff coverage is 73.2%.

@@           Coverage Diff            @@
##           master    #266     +/-   ##
========================================
- Coverage    94.8%   92.3%   -2.5%     
========================================
  Files          21      21             
  Lines        1321    1453    +132     
========================================
+ Hits         1253    1342     +89     
- Misses         68     111     +43     
Impacted Files Coverage Ξ”
cloudpathlib/anypath.py 76.1% <71.8%> (-23.9%) :arrow_down:
cloudpathlib/cloudpath.py 93.5% <100.0%> (ΓΈ)
cloudpathlib/s3/s3client.py 92.9% <0.0%> (-2.7%) :arrow_down:
cloudpathlib/gs/gsclient.py 92.8% <0.0%> (-1.8%) :arrow_down:

codecov-commenter avatar Aug 19 '22 18:08 codecov-commenter

Any opinion on this @pjbull or @jayqi? I'm happy to make this a polished PR or go after another solution if you have any suggestions.

chbehrens avatar Sep 11 '22 19:09 chbehrens

Sorry @chbehrens. Delay is because, as you point out, the current implementation is a lot of boilerplate which seems less than ideal just to support use of AnyPath as a type hint (does typing things as Union[CloudPath, Path] instead work for your purposes?). I don't have a better implementation off the top of my head, and like you say we should think about it in the context of our overall class hierarchy. Happy to open an issue for discussion, but I'm not super keen on this PR as is. Maybe @jayqi has a different opinion.

pjbull avatar Sep 12 '22 14:09 pjbull

Don't get me wrong, I'm also not keen on it the way it is. I just did it to show what I'd like to achieve in the end behavior-wise but preferably via something more meta programming like. That's why I made it a draft and was wondering whether you have some ideas or what you think of the overall intended behavior. I'm sorry if it's the wrong format here, I just thought a draft PR might demonstrate it better than an issue.

chbehrens avatar Sep 13 '22 07:09 chbehrens

@ringohoffman Do you have thoughts on this after implementing your changes in #298? Is there a cleaner/easier way to get useful type hinting for AnyPath without duplicating all the signatures?

pjbull avatar Dec 18 '22 23:12 pjbull

I was asking about this over at https://github.com/microsoft/pyright/issues/4331 and got back a few suggestions... and the virtual superclass approach was not one of them. I will have to play around to see which results in the most expected / convenient typing experience.

ringohoffman avatar Dec 20 '22 07:12 ringohoffman

Much appreciated! Curious what you discover.

It does some interesting stuff in regard to typing.

πŸ˜† πŸ˜† πŸ˜†

pjbull avatar Dec 20 '22 17:12 pjbull

@chbehrens I'm going through old PRs to keep them moving.

I think that I've come around on this. I was hoping for some cleaner upstream fix that makes this kind of polymorphic class work, but I don't think there's another way to get useful completions for AnyPath until something like #347 comes in upstream in Python (and we can subclass from that).

Would you be willing to rebase this on the latest and add tests/docs?

Outside of that, I am curious if one of the other ideas in this thread might work: https://github.com/microsoft/pyright/issues/4331#issuecomment-1350209969

It does seem that Protocol is potentially a friendlier implementation than ABC since it is slightly less boilerplate and plays nicer with static type checkers, but I'm not sure it solves all our issues without some experimentation.

pjbull avatar Jul 22 '23 18:07 pjbull

I'll try to have a look at it if I find time but let's just close this, an issue would be better suited to track the topic if necessary.

chbehrens avatar Jul 25 '23 07:07 chbehrens

I've experimented with Protocols, and one of the main problems IIRC is that the Pydantic validation methods were getting caught up in it. There's also some annoying stuff with API changes to Path over different Python versions, and not being able to match all of them.

jayqi avatar Jul 25 '23 14:07 jayqi