drivendataorg / cloudpathlib

Python pathlib-style classes for cloud storage services such as Amazon S3, Azure Blob Storage, and Google Cloud Storage.
https://cloudpathlib.drivendata.org
MIT License
478 stars 62 forks source link

use variable for client schemes, allowing override #467

Closed kujenga closed 2 months ago

kujenga commented 2 months ago

This change is intended to make the default client implementations more flexible so that their scheme can be customized. This can be useful in scenarios where a subclass wants to implement a custom scheme on e.g. a S3 compatible API [1] but with a custom scheme so that the default S3 access is still also available.

[1] https://cloudpathlib.drivendata.org/stable/authentication/#accessing-custom-s3-compatible-object-stores

The tests have been updated to include a new s3-like rig which uses the new scheme override functionality.

(If there is a better strategy for tests or this seems like overkill let me know and happy to adjust! It seems a bit more work is needed to get them passing)

Closes #466


Contributor checklist:

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.2%. Comparing base (b776bee) to head (147db5b). Report is 1 commits behind head on 467-live-tests.

Files with missing lines Patch % Lines
cloudpathlib/azure/azblobclient.py 75.0% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## 467-live-tests #467 +/- ## ================================================ - Coverage 94.2% 93.2% -1.0% ================================================ Files 23 23 Lines 1745 1745 ================================================ - Hits 1645 1628 -17 - Misses 100 117 +17 ``` | [Files with missing lines](https://app.codecov.io/gh/drivendataorg/cloudpathlib/pull/467?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=drivendataorg) | Coverage Δ | | |---|---|---| | [cloudpathlib/gs/gsclient.py](https://app.codecov.io/gh/drivendataorg/cloudpathlib/pull/467?src=pr&el=tree&filepath=cloudpathlib%2Fgs%2Fgsclient.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=drivendataorg#diff-Y2xvdWRwYXRobGliL2dzL2dzY2xpZW50LnB5) | `90.0% <100.0%> (-1.5%)` | :arrow_down: | | [cloudpathlib/s3/s3client.py](https://app.codecov.io/gh/drivendataorg/cloudpathlib/pull/467?src=pr&el=tree&filepath=cloudpathlib%2Fs3%2Fs3client.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=drivendataorg#diff-Y2xvdWRwYXRobGliL3MzL3MzY2xpZW50LnB5) | `92.9% <ø> (-1.5%)` | :arrow_down: | | [cloudpathlib/azure/azblobclient.py](https://app.codecov.io/gh/drivendataorg/cloudpathlib/pull/467?src=pr&el=tree&filepath=cloudpathlib%2Fazure%2Fazblobclient.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=drivendataorg#diff-Y2xvdWRwYXRobGliL2F6dXJlL2F6YmxvYmNsaWVudC5weQ==) | `89.9% <75.0%> (-6.9%)` | :arrow_down: |
pjbull commented 2 months ago

Also, one other thought: I think adding a rig for this is overkill. That will duplicate all the tests, which isn't necessary. I'd just do a single test with the custom scheme to show that piece works.

kujenga commented 2 months ago

For all the instances you replaced, you should be able to get the cloud_prefix from the instance of the cloud_path passed to the method, no?

Good call, done!

Also, one other thought: I think adding a rig for this is overkill. That will duplicate all the tests, which isn't necessary. I'd just do a single test with the custom scheme to show that piece works.

Fair enough, done!

If there's anything else just let me know! I left them as separate commits to keep what I changed clear but happy to squash as well if you prefer.

kujenga commented 2 months ago

great, let me know if there's anything else!

kujenga commented 1 month ago

Hi @pjbull , any chance you'd be able to tag a release with this new functionality? Thank you!

pjbull commented 1 month ago

@kujenga Yep! Need to get a release out for 3.13 anyway in the next few days.