rmdir and rmtree #52
rmdir and rmtree #52
Conversation
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
==========================================
- Coverage 81.51% 81.23% -0.29%
==========================================
Files 9 9
Lines 622 634 +12
==========================================
+ Hits 507 515 +8
- Misses 115 119 +4
Continue to review full report at Codecov.
|
| @@ -508,6 +519,7 @@ def download_to(self, destination: Union[str, os.PathLike]): | |||
| if self.is_file(): | |||
| self.backend._download_file(self, destination) | |||
| else: | |||
| destination.mkdir(exist_ok=True) | |||
There was a problem hiding this comment.
Will we need parents=True as well? I think if a user specifies a path we can assume they want the whole thing to be created.
There was a problem hiding this comment.
I guess that's a design decision to make on whether we want to change the behavior of this function. The existing behavior of download_to if the source is a file also will not create parents, which is consistent with typical behavior for Path file methods (though it is inconvenient sometimes).
There was a problem hiding this comment.
That is doubly true. First, that pathlib never creates dirs for you, and second that it is inconvenient annoying. In a bunch of our projects we've got a repeated path.parent.mkdir(exist_ok=True, parents=True) before each path write.
I don't feel strongly either way. I didn't track down a reason for pathlib not creating dirs in some cursory searching of the Python bug tracker / mailing list, and I would be curious to see discussion why file creation methods (like write_text for example) wouldn't create the full path. I guess it's not explicitly saying to create a directory, but Path("asdf/asdf/asdf.txt").write_text("asdf") does seem pretty explicit...
In the absence of a reason not to, I'm inclined to create directories since that feels more useful and more like the API I would want to use in my development experience, but happy to punt the discussion to an issue as well.
I guess that's a design decision to make on whether we want to change the behavior of this function. The existing behavior of download_to if the source is a file also will not create parents
This is a good point that highlights that throughout the project we have contracts that are under-specified. It may be the case that the backend _download_file actually does create non-existing parent directories since we don't explicitly stop it from doing so. We haven't defined if Backend._download_file should or should not create parent directories, and we don't enforce it across backends. This is the kind of place where we're going to want to get more explicit in documenting these contracts and what is expected so that we mitigate differences across backend implementations.
I'll file an issue to track creating a backend implementation guide that specifies a handful of these things.
(Also, I will say that the new functions in cloudpathlib are the make-it-work implementations for a happy path. They definitely have not considered every use case and corner of API consistency. We should feel free to change them to make them better while we're developing.)
There was a problem hiding this comment.
I guess my inclination is to punt for now. Given that the precedent is to not create directories for you automatically, not just for pathlib, but also for shells like bash, I think it makes sense to standardize at first on that. If we decide otherwise, I think we should make a concerted effort to do that consistently everywhere in cloudpathlib, and also test for it everywhere.
| @@ -179,3 +181,15 @@ def paginate(self, Bucket=None, Prefix="", Delimiter=None): | |||
| dirs = [{"Prefix": str(_.relative_to(self.root))} for _ in page if _.is_dir()] | |||
| files = [{"Key": str(_.relative_to(self.root))} for _ in page if _.is_file()] | |||
| yield {"CommonPrefixes": dirs, "Contents": files} | |||
|
|
|||
|
|
|||
| def delete_empty_parents_up_to_root(path, root): | |||
There was a problem hiding this comment.
Now we act like a true blob store!
CloudPath.rmdirnow errors if directory is not emptyCloudPath.rmtreethat recursively deletes files, named aftershutil.rmtreemkdirfor cloud providers that support creating directories #51.