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: use native exists() method in GSClient #420

Merged
merged 6 commits into from
Apr 5, 2024

Conversation

bachya
Copy link
Contributor

@bachya bachya commented Apr 4, 2024

SSIA

Closes #356


Contributor checklist:

  • I have read and understood CONTRIBUTING.md
  • Confirmed an issue exists for the PR, and the text Closes #issue appears in the PR summary (e.g., Closes #123).
  • Confirmed PR is rebased onto the latest base
  • Confirmed failure before change and success after change
  • Any generic new functionality is replicated across cloud providers if necessary
  • Tested manually against live server backend for at least one provider
  • Added tests for any new functionality
  • Linting passes locally
  • Tests pass locally
  • Updated HISTORY.md with the issue that is addressed and the PR you are submitting. If the top section is not `## UNRELEASED``, then you need to add a new section to the top of the document for your change.

Copy link
Member

@pjbull pjbull left a comment

Choose a reason for hiding this comment

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

Thanks @bachya!

One change here. Can you also confirm that you tested the case in #356 with a totally empty bucket and this method works?

cloudpathlib/gs/gsclient.py Outdated Show resolved Hide resolved
@bachya
Copy link
Contributor Author

bachya commented Apr 4, 2024

@pjbull Confirmed, I have this working on an empty GCP bucket.

@pjbull
Copy link
Member

pjbull commented Apr 4, 2024

Thanks @bachya, happy to merge this once the code quality and mocked tests are passing.

@pjbull
Copy link
Member

pjbull commented Apr 5, 2024

Tests need an update in the mocked version (which mock the cloud SDKs):

More info on running the test suite here: https://cloudpathlib.drivendata.org/stable/contributing/#tests

@bachya
Copy link
Contributor Author

bachya commented Apr 5, 2024

Thanks, @pjbull. I couldn't see the results until you approved the CI run; got them now, and I'll update the tests.

@pjbull
Copy link
Member

pjbull commented Apr 5, 2024

@bachya just fyi, you can run make lint and make test locally so you don't have to wait for the CI runs.

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.4%. Comparing base (c9da3f3) to head (3e16d8f).

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #420     +/-   ##
========================================
- Coverage    93.8%   93.4%   -0.4%     
========================================
  Files          23      23             
  Lines        1644    1639      -5     
========================================
- Hits         1543    1532     -11     
- Misses        101     107      +6     
Files Coverage Δ
cloudpathlib/gs/gsclient.py 91.4% <100.0%> (-1.7%) ⬇️

... and 2 files with indirect coverage changes

@pjbull
Copy link
Member

pjbull commented Apr 5, 2024

Looks good! I'm going to merge into a repo-local branch so we can run against the live server backends, which is not possible on PRs from forks.

@pjbull pjbull changed the base branch from master to 420-live-tests April 5, 2024 04:41
@pjbull pjbull merged commit b228c39 into drivendataorg:420-live-tests Apr 5, 2024
21 of 23 checks passed
pjbull added a commit that referenced this pull request Apr 5, 2024
* fix: use native `exists()` method in `GSClient`

* update HISTORY.md

* only short-circuit

* fix lint

* update tests

* lint and tests

Co-authored-by: Aaron Bach <bachya1208@gmail.com>
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.

exists(gs://bucket/) fails with StopIteration on an empty bucket
2 participants