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

Workaround for pydantic v2 #288

Merged
merged 2 commits into from Aug 10, 2023
Merged

Conversation

xl0
Copy link
Contributor

@xl0 xl0 commented Aug 1, 2023

It's a bit heavier than expected to make mypy happy on both pydantic versions.

Copy link
Collaborator

@mesozoic mesozoic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! 🙌 Thank you for figuring this out!

I like this approach because it is agnostic which version our library users require for other parts of their project. Also, since it does not require changing any of our APIs or behaviors, we can make it a patch release instead of a minor version bump.

I suggested a couple changes below. I also think we could add a few lines to tox.ini to ensure that our test suite runs successfully against both major versions of pydantic:

diff --git a/tox.ini b/tox.ini
index 584ae4d..142db2e 100644
--- a/tox.ini
+++ b/tox.ini
@@ -3,6 +3,7 @@ envlist =
     pre-commit
     mypy
     py3{8,9,10,11}-requests{min,max}
+    py3{8,9,10,11}-pydantic1
     coverage
 
 [gh-actions]
@@ -29,6 +30,7 @@ deps =
     -r requirements-test.txt
     requestsmin: requests==2.22.0  # Keep in sync with setup.cfg
     requestsmax: requests>=2.22.0  # Keep in sync with setup.cfg
+    pydantic1: pydantic<2  # Lots of projects still use 1.x
 
 [testenv:coverage]
 passenv = COVERAGE_FORMAT

I'll be very happy to merge this branch once we've got test coverage and have at least tried those other ideas.

pyairtable/models/_base.py Show resolved Hide resolved
tests/test_api_types.py Outdated Show resolved Hide resolved
pyairtable/api/types.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 5, 2023

Codecov Report

Merging #288 (762b1bc) into main (d775b64) will decrease coverage by 0.29%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
- Coverage   95.80%   95.51%   -0.29%     
==========================================
  Files          19       20       +1     
  Lines         929      937       +8     
==========================================
+ Hits          890      895       +5     
- Misses         39       42       +3     
Files Changed Coverage Δ
pyairtable/_compat.py 62.50% <62.50%> (ø)
pyairtable/api/types.py 100.00% <100.00%> (ø)
pyairtable/models/_base.py 97.77% <100.00%> (ø)

@xl0
Copy link
Contributor Author

xl0 commented Aug 6, 2023

Agree re tox, I'll add it later today.

@xl0
Copy link
Contributor Author

xl0 commented Aug 8, 2023

@mesozoic I updated the PR to

  • Add -pydantic1 to tox tests. Also added mypy-pydantic1.
  • Moves pydantic stuff into pyairtable/pydantic.py. Can't be in utils due to circular dependency.

@xl0
Copy link
Contributor Author

xl0 commented Aug 8, 2023

@mesozoic , any idea what's up with the docs failing?

@mesozoic
Copy link
Collaborator

mesozoic commented Aug 8, 2023

@mesozoic , any idea what's up with the docs failing?

I think you might've forgotten to git add the new file. Can we call it _compat.py, though, (1) to make it clear that it's internal and not part of the public API, and (2) to avoid any confusion arising from the module and its contents having the same name?

@xl0
Copy link
Contributor Author

xl0 commented Aug 8, 2023

@mesozoic , heh, my bad. Ok, renamed it to _compat.py.

@mesozoic
Copy link
Collaborator

mesozoic commented Aug 9, 2023

@xl0 One more request, could you rebase the branch with signed commits? Easiest way is with SSH, in my experience, but the repo requires it before we merge. I could rebase and merge it on your behalf, but then you won't get any credit. 😁

@xl0
Copy link
Contributor Author

xl0 commented Aug 9, 2023

Ok, I'll check it later today.

@xl0
Copy link
Contributor Author

xl0 commented Aug 9, 2023

@mesozoic , done

@mesozoic mesozoic merged commit 2503060 into gtalarico:main Aug 10, 2023
8 checks passed
@mesozoic
Copy link
Collaborator

Thanks very much for solving this!

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.

None yet

2 participants