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 Support #49

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Pydantic v2 Support #49

wants to merge 7 commits into from

Conversation

Jypear
Copy link

@Jypear Jypear commented Jul 28, 2023

Creating this as a work in progress/draft PR to get the ball rolling on the pydanticv2 migrations.
There's potentially a breaking change (changing the model_orm argument for SQLRepo to just orm). Using "model_" now clashes with a pydantic protected Namespace.
We might be able to get round this using a Field alias, but it would require some testing to see if it would still cause the clash.

Tests are still failing at this point but Memory Repo and SQLRepo are running in a basic test.

Will continue to work on this over the weekend hopefully.

- replace use of __fields__ with model_fields
- change variabled model_orm to just orm to prevent class with pydantic protected name space
- Change looking at type_ to annotation
- remove const =True from field arguments
- use a context manager to change working directory to work with pydantic pathlib type better
- move SQL tests away from config class
- migraste orm_mode to from_attributes
@Jypear Jypear mentioned this pull request Sep 18, 2023
Josh Pearson added 4 commits October 2, 2023 23:39
- Added more =None to prevent validation errors on optional fields
- Changed raises and warn test to validate/test malformed integers instead of strings
	- This is due to a change in pydanticv2 no longer being able to convert strings to integers
- Changed some pytest to use tmp_path fixture instead of tmpdir due to pydanticv2 compatibility with pathlib
- more setting None as default values in optional[str] fields
- is_required will now only correctly set on Optional and Union fields if a default argument of None is specified
	- To fix this, added an if statement in text to look for the tests with Optional or Union and change the class with a default arg for those tests
- Functionality has changed with having a nested literal inside an Optional type
	- updated the to sql type function to check if the type is still Literal when checking Optional or Union, if it is then perform the Literal parsing on the nested type
	- remove final usage of .dict() for .model_dump()
@Jypear Jypear marked this pull request as ready for review October 6, 2023 17:35
@Jypear
Copy link
Author

Jypear commented Oct 6, 2023

Hi @Miksus,

Just to let you know I've now marked this PR ready for review.
I've got rid of all failing tests and deprecation warnings (bar one, I have no idea where this is coming from).
There will also be some outstanding issues, this project uses pydantic-sqlalchemy, which ties pydantic to version 1. There is a PR which I have dropped a comment on to see if that can get pushed out.

Breaking changes

This PR contains breaking changes for anybody that consumes this, so you may have to version this appropriately.

  • 'model_orm' has been changed to 'orm' in the SQLRepo. This is because 'model_' is now a protected namespace inside pydantic.
  • This is yet to be tested, but due to the way that pydantic now handles the Optional type and default arguments, I'm not sure if that will have a lasting affect on optional fields inside Repos

The rest are just migration pieces.
Please feel free to pull me up on anything in a review when you get chance.

@gregoryfoster
Copy link

@Jypear, thanks for your work on this, I've switched to your fork for my projects. To avoid the model_orm breaking change, have you considered removing model_ from the set of Pydantic protected namespaces on the SQLRepo class?
pydantic/pydantic#7121 (comment)

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