-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[1.10.X] Fix field regex with StrictStr type annotation #4538
[1.10.X] Fix field regex with StrictStr type annotation #4538
Conversation
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.
otherwise happy to accept this.
pydantic/types.py
Outdated
regex = cls._regex() | ||
if regex: | ||
if not regex.match(value): |
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'm afraid this might be slow since the compiled regex might not be cached.
Better to do the following which I think should work in both cases
regex = cls._regex() | |
if regex: | |
if not regex.match(value): | |
if cls.regex: | |
if not re.match(cls.regex, value): |
pydantic/types.py
Outdated
strict = False | ||
|
||
@classmethod | ||
def __modify_schema__(cls, field_schema: Dict[str, Any]) -> None: | ||
regex = cls._regex() |
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.
you'll therefore still need an isinstance
check here.
@samuelcolvin Thanks for your review feedback! I think I've made the changes accordingly which should improve regex caching. Happy to receive your feedback again. 🙂 |
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.
@hramezani please review and check you're happy with this.
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 👍
Thanks @sisp
thanks so much for this. |
Sure thing, thanks for the review parallel to all the v2 dev work! Can't wait to see v2 and all its improvements! 😉 |
There's an inconsistency between
constr(...)
andConstrainedStr
, which is the base class ofStrictStr
, with regard to the expected type of theregex
argument/attribute which causes a field of typeStrictStr
with aregex
constraint specified via theField(...)
class to raise an error:I've fixed this inconsistency in a backwards compatible way by extending the type of
regex
inConstrainedStr
to also support astr
value in addition toPattern[str]
.I hope this fix will be considered despite the ongoing efforts towards Pydantic v2. 🙂