-
Notifications
You must be signed in to change notification settings - Fork 64
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
Pydantic v2 update #349
Pydantic v2 update #349
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #349 +/- ##
========================================
- Coverage 94.2% 93.6% -0.6%
========================================
Files 22 22
Lines 1484 1507 +23
========================================
+ Hits 1399 1412 +13
- Misses 85 95 +10
|
@@ -90,6 +90,9 @@ def copy_from(self, CopySource=None, Metadata=None, MetadataDirective=None): | |||
|
|||
def download_file(self, to_path, Config=None, ExtraArgs=None): | |||
to_path = Path(to_path) | |||
|
|||
to_path.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to address MacOS test failures:
https://github.com/drivendataorg/cloudpathlib/actions/runs/5366826109/jobs/9736473309
I couldn't get a root cause why that was the only scenario of cloud provider/os to fail and why it started happening without a code change on our part. We actually already do this creation in the Mock for GCS, and if it were a problem with the test code not making all the directories in the ways end users would need to, I would expect the live tests to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I wonder if we could get the Annotated version of this to work, because I recall that adding these methods makes AnyPath
not work as a protocol class for type checking with CloudPath
and Path
.
Yeah, it does seem like it should be possible, but IIRC it wasn't playing nice with the current |
Note: This is on top of #348 and should be merged after that. Commits from65d7911
have the relevant changes.Updated to be an incremental change without needing different pydantic v1/v2 compatible versions.
Update for compatibility with pydantic v2. Since the two relevant method names changed entirely, we can keep v1/v2 compatibility side by side.
Manually ran test suite on pydantic 1.10 and 2.0
Tried a few other implementations from the docs like using
Annotated
and also a separate mixin class, but ultimately this implementation, which is a minimal set of changes from our v1 implementation, is the one that played best with our dispatch/polymorphism (and didn't require any public API changes).