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

Fix nested remove for azure and gcs #185

Merged
merged 1 commit into from
Dec 29, 2021
Merged

Fix nested remove for azure and gcs #185

merged 1 commit into from
Dec 29, 2021

Conversation

pjbull
Copy link
Member

@pjbull pjbull commented Dec 29, 2021

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
Contributor

@codecov
Copy link

codecov bot commented Dec 29, 2021

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
Copy link
Member Author

pjbull commented Dec 29, 2021

@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
Copy link
Member

jayqi commented Dec 30, 2021

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