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.

Sign up for GitHub

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

Jump to bottom

Add md5 hash property to GSPath #483

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add md5 hash property to GSPath #483

wants to merge 11 commits into from

Conversation

fafnirZ
Copy link

@fafnirZ fafnirZ commented Oct 31, 2024 •
edited
Loading

Adding md5 property into GSPath
Updating supported methods and properties in repository root README
Updating LocalGSPath to mimic LocalAzureBlobPath, in that there exists md5 property
Updating mock_gs.MockBlob to include md5_hash property

Closes #482

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.

@pjbull
Copy link
Member

pjbull commented Oct 31, 2024

Thanks @fafnirZ! Could you CI failures and add a new test?

@codecov Codecov
Copy link

codecov bot commented Nov 1, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.1%. Comparing base (7f1a5dc) to head (2ea689a).

Files with missing lines Patch % Lines
cloudpathlib/gs/gspath.py 85.7% 1 Missing ⚠️
cloudpathlib/local/implementations/gs.py 66.6% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##           master    #483     +/-   ##
========================================
- Coverage    94.4%   93.1%   -1.4%     
========================================
  Files          23      23             
  Lines        1776    1785      +9     
========================================
- Hits         1678    1663     -15     
- Misses         98     122     +24
Files with missing lines Coverage Δ
cloudpathlib/gs/gsclient.py 90.0% <ø> (-1.5%) ⬇️
cloudpathlib/gs/gspath.py 92.4% <85.7%> (-1.2%) ⬇️
cloudpathlib/local/implementations/gs.py 97.1% <66.6%> (-2.9%) ⬇️

... and 2 files with indirect coverage changes

@fafnirZ
Copy link
Author

fafnirZ commented Nov 2, 2024

Hey @pjbull, I'm kinda confused as to why my changes has caused so many seemingly unrelated tests to fail on the live server, any chance you could point me in the right direction?

@pjbull
Copy link
Member

pjbull commented Nov 2, 2024

Sorry, as noted here, those are expected since PRs from forks can't access the credentials:
https://cloudpathlib.drivendata.org/stable/contributing/#pr-cicd-test-run

@fafnirZ
Copy link
Author

fafnirZ commented Nov 2, 2024 •
edited
Loading

Thanks for the response!
Ohhh I missed that section, my bad haha. was getting worried for a second and was thinking I made a big mistake😅.
All goods then! everything seems to look fine then I think?

Lmk if there are any issues with this PR! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GSPath md5 hash property could potentially be supported
2 participants
@fafnirZ @pjbull