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

Pydantic v2 update #349

Merged
merged 8 commits into from
Jul 12, 2023
Merged

Pydantic v2 update #349

merged 8 commits into from
Jul 12, 2023

Conversation

pjbull
Copy link
Member

@pjbull pjbull commented Jul 10, 2023

Note: This is on top of #348 and should be merged after that. Commits from 65d7911 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).

@pjbull pjbull requested a review from jayqi July 10, 2023 00:45
@github-actions
Copy link
Contributor

github-actions bot commented Jul 10, 2023

@github-actions github-actions bot temporarily deployed to pull request July 10, 2023 00:46 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 10, 2023 00:56 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 10, 2023 00:58 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 10, 2023 02:42 Inactive
@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Merging #349 (6be8a17) into master (2a3d712) will decrease coverage by 0.6%.
The diff coverage is 83.3%.

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     
Impacted Files Coverage Δ
cloudpathlib/cloudpath.py 91.6% <80.0%> (-1.0%) ⬇️
cloudpathlib/anypath.py 90.0% <85.7%> (-10.0%) ⬇️

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

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.

Copy link
Member

@jayqi jayqi left a 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.

@pjbull
Copy link
Member Author

pjbull commented Jul 11, 2023

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 ABC implementation of AnyPath.

@github-actions github-actions bot temporarily deployed to pull request July 12, 2023 16:45 Inactive
@pjbull pjbull merged commit 6b4afe9 into master Jul 12, 2023
24 checks passed
@pjbull pjbull deleted the pydantic-v2-update branch July 12, 2023 17:14
@pjbull pjbull mentioned this pull request Jul 22, 2023
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.

2 participants