Skip to content

Fix nested remove for azure and gcs#185

Merged
pjbull merged 1 commit into
masterfrom
184-rmtree-nested
Dec 29, 2021
Merged

Fix nested remove for azure and gcs#185
pjbull merged 1 commit into
masterfrom
184-rmtree-nested

Conversation

@pjbull

@pjbull pjbull commented Dec 29, 2021

Copy link
Copy Markdown
Member

Closes #184

Problem is that _list_dir implementations return both directories and files, but our _remove function for both of these clients assumes only files.

Added regression test which fails on Azure + GCS without this fixes, but passes with the fix.

Updated history and version to cut new release with this fix

@pjbull
pjbull requested a review from jayqi December 29, 2021 18:18
@github-actions

Copy link
Copy Markdown
Contributor

@codecov

codecov Bot commented Dec 29, 2021

Copy link
Copy Markdown

Codecov Report

Merging #185 (ba45cfb) into master (3901746) will not change coverage.
The diff coverage is 100.0%.

@@          Coverage Diff           @@
##           master    #185   +/-   ##
======================================
  Coverage    94.1%   94.1%           
======================================
  Files          21      21           
  Lines        1189    1189           
======================================
  Hits         1120    1120           
  Misses         69      69           
Impacted Files Coverage Δ
cloudpathlib/azure/azblobclient.py 94.6% <100.0%> (ø)
cloudpathlib/gs/gsclient.py 94.0% <100.0%> (ø)

@pjbull

pjbull commented Dec 29, 2021

Copy link
Copy Markdown
Member Author

@jayqi Going to merge this since I think this is low-risk and well-tested so that we can cut a release to fix #184.

Happy to address any code review comments in separate PR.

@pjbull
pjbull merged commit d08f18c into master Dec 29, 2021
@pjbull
pjbull deleted the 184-rmtree-nested branch December 29, 2021 18:44
@jayqi

jayqi commented Dec 30, 2021

Copy link
Copy Markdown
Member

Looks okay. The main thing that stands out to me is that this may have a performance impact from additional network calls.

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.

rmtree not working when there is directory inside (google cloud storage)

2 participants