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 integration and AnyPath polymorphic class #130

Merged
merged 8 commits into from
Mar 7, 2021

Conversation

jayqi
Copy link
Member

@jayqi jayqi commented Mar 7, 2021

Question:

  • For now, I've just added pydantic to requirements-dev.txt for the tests. Should we make this an extras option instead? There's no direct runtime dependency by cloudpathlib on pydantic. You'd only actually need pydantic if you try to make a model and need to import BaseModel.

Note:

  • mypy has its own subtype-checking logic that does not use issubclass, so the AnyPath implementation doesn't quite pass mypy. I'm currently just putting in some # type: ignores, but one may consider this implementation not-type-safe shenanigans. I may open an issue on mypy later to ask about if there is a recommended way to do things.

@jayqi jayqi requested a review from pjbull March 7, 2021 01:33
@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #130 (3d542de) into master (0eed4e9) will increase coverage by 0.2%.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master    #130     +/-   ##
========================================
+ Coverage    92.0%   92.2%   +0.2%     
========================================
  Files          19      20      +1     
  Lines        1066    1098     +32     
========================================
+ Hits          981    1013     +32     
  Misses         85      85             
Impacted Files Coverage Δ
cloudpathlib/__init__.py 100.0% <100.0%> (ø)
cloudpathlib/anypath.py 100.0% <100.0%> (ø)
cloudpathlib/cloudpath.py 91.0% <100.0%> (+0.1%) ⬆️

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2021

@pjbull
Copy link
Member

pjbull commented Mar 7, 2021

For now, I've just added pydantic to requirements-dev.txt for the tests. Should we make this an extras option instead? There's no direct runtime dependency by cloudpathlib on pydantic. You'd only actually need pydantic if you try to make a model and need to import BaseModel.

Agreed to leave it out of the explicit requirements and just use it in dev.

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.

This is 🔥 🔥 🔥

One thought is I don't think this implementation will support Pure paths. I'm fine opening an issue and deciding how we want to handle that later, but just wanted to mention.

docs/docs/anypath-polymorphism.md Outdated Show resolved Hide resolved
docs/docs/anypath-polymorphism.md Show resolved Hide resolved
@jayqi
Copy link
Member Author

jayqi commented Mar 7, 2021

I thought of the PurePath thing too, and I decided it makes sense not to support it, at least right now.

  • We don't have any PureCloudPath classes. IIRC, use of cloudpathlib without authentication is undocumented and not explicitly supported.
  • I don't think we can automatically distinguish between inputs to PurePath and Path for dispatch. We'd have to provide more information and we'd have to invent an interface for it.
  • Most practical use cases that need to switch between cloud paths and local file paths probably care about interacting with real files and not a hypothetical filesystem.

@jayqi jayqi requested a review from pjbull March 7, 2021 02:31
@pjbull pjbull merged commit 3d6dc72 into master Mar 7, 2021
@pjbull pjbull deleted the jyq-pydantic-anypath branch March 7, 2021 05:50
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.

Add pydantic validator support Add AnyPath class that will dispatch to either local or cloud
2 participants