-
-
Notifications
You must be signed in to change notification settings - Fork 768
✨ Support pattern
parameter
#1231
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
base: main
Are you sure you want to change the base?
Conversation
regex
to param
to provide compatibility with Pydantic V2
Thanks for the PR! We're going through the backlog of PRs right now, and will get back to you once we've had the time to review this and consider the (breaking) change 🙏 |
@svlandeg Nice to hear you are taking action on this matter. There is remark, the parameter the was renamed between Pydantic v1 and v2 is The suggestion I made in the PR was to keep both versions compatible when upgrading, but there is an alternative, which is not managing the change between them and just provide the proper type hints. If you want me to update the PR just let me know, IDK which approach is more adequate for the project. |
This comment was marked as resolved.
This comment was marked as resolved.
regex
to param
to provide compatibility with Pydantic V2regex
to pattern
to provide compatibility with Pydantic V2
This comment was marked as resolved.
This comment was marked as resolved.
Since we still support Also, I think we should mark Still we need to add some test:
|
Noted @YuriiMotov, I will do that. I would like to propose the following change: def Field(default, *, ..., **schema_extra) rather than def Field(default, *, ..., schema_extra) |
a5e8961
to
0b9f96f
Compare
I think this should be done in a separate PR. And I would do it in a different way - I would add Could you please address other ideas from this comment? |
Regarding this, I would still label it as Tests
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
regex
to pattern
to provide compatibility with Pydantic V2pattern
parameter
This comment was marked as resolved.
This comment was marked as resolved.
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.
LGTM in general, just one suggestion:
I still think we should deprecate
regex
parameter and prioritizepattern
regardless the version of Pydantic installed.
Let's leave this decision to Sebastian
sqlmodel/main.py
Outdated
if IS_PYDANTIC_V2: | ||
current_schema_extra.update(pattern=pattern or regex) | ||
else: | ||
current_schema_extra.update(regex=regex or pattern) |
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.
I still think we should deprecate regex
parameter and prioritize pattern
regardless the version of Pydantic installed.
Let's leave this decision to Sebastian
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.
I would leave it like this, separate but not deprecate any, it is the main purpose of this PR, to support both and after migrating from pydantic
v1
to v2
it remains compatible
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.
It's just a bit confusing that we will have 2 duplicating parameters.
Pydantic has marked regex
as deprecated in V2
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.
Yes, that's true, but since sqlmodel
supports both versions we should keep compatibility.
Otherwise this PR would be just renaming the parameter
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.
We support both, but mark old one as deprecated. This way we give users time to update their code bases to use new parameter instead of old one.
Later we will drop regex
and only allow pattern
regardless the Pydantic version.
I think we should go this way
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.
Ok. Then:
- Support both parameters (like it is)
- In future versions
schema_extra
andregex
will be removed in order to support justpattern
as pydantic have done
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.
Yes, but schema_extra
will be just renamed to json_schema_extra
and schema_extra
will be marked as deprecated
Tests currently fail because new Pydantic version has been released and caused linting errors. See: #1591 |
I faced this issue with Pydantic V2 and there is a simple workaround to it by passing the
pattern
Field parameter as schema extra:Ex:
I don't know if this issue was already fixed in another branch, I saw there was a temporary PR already made but closed.