-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add optional field argument to __modify_schema__
#3434
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
Conversation
pydantic/schema.py
Outdated
| @@ -87,6 +87,16 @@ | |||
| TypeModelSet = Set[TypeModelOrEnum] | |||
|
|
|||
|
|
|||
| def _apply_modify_schema(modify_schema: Callable, field: ModelField, schema: Dict[str, Any]): | |||
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.
The field should be Optional[ModelField] and not ModelField, since __modify_schema__() is not only called for fields. If it's called for entire model, there is no ModelField in this context.
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.
Fixed in newer revision ☝️
e38663c to
add958c
Compare
field argument to __modify_schema__field argument to __modify_schema__
|
please review |
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 LGTM.
docs/usage/schema.md
Outdated
| You can also add any of the following arguments to the signature to use them in the implementation: | ||
|
|
||
| * `field`: the field whose schema is customized. Type of object is `pydantic.fields.ModelField`. | ||
| * `**kwargs`: if provided, the above argument will be provided in they `kwargs` dictionary |
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 can also add any of the following arguments to the signature to use them in the implementation: | |
| * `field`: the field whose schema is customized. Type of object is `pydantic.fields.ModelField`. | |
| * `**kwargs`: if provided, the above argument will be provided in they `kwargs` dictionary | |
| `__modify_schema__` can also take a `field` argument which will have type `Optional[ModelField]`, pydantic will inspect the signature of `__modify_schema__` to determine | |
| whether the `field` argument should be included. |
Or something. Explaining about **kwargs is overkill here I think.
Might also be worth adding an example.
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.
Simplified the documentation as suggested, and added an example
tests/test_schema.py
Outdated
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| sys.version_info < (3, 7), reason='schema generation for generic fields is not available in python < 3.7' |
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.
The test doesn't need to use a generic, so can remove the skipif
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.
If there's a good reason to add a test for a generic here too, you can do that as a separate test.
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.
No there's no reason for it to be generic, so changed the test to work without them
tests/test_schema.py
Outdated
|
|
||
| class GenModel(Generic[T]): | ||
| @classmethod | ||
| def __modify_schema__(cls, field_schema, field): |
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.
| def __modify_schema__(cls, field_schema, field): | |
| def __modify_schema__(cls, field_schema, field): | |
| assert isinstance(field, ModelField) |
|
please update |
add958c to
181cf43
Compare
|
Thank you for the suggestions. I updated and rebased the PR accordingly. please review |
|
thanks so much. |
Change Summary
Add optional
fieldparameter tomodel.__modify_schema__()to opt in to receiving theModelFieldobject of the field. This makes it possible e.g. for the implementation of__modify_schema__()of generic type to know the runtime type of its subfields.The concept is inspired by the optional parameters in validators, implemented in the
pydantic.class_validatorsmodule. There are some differences:fieldis supported.valuesdoes not make sense in this context, since validation deals with concrete object, and this with the class itself. Modelconfigmight be supported, but is not included in this PR.fieldisOptional[ModelField]as opposed toModelField, unlike validators. This is because theschemamethods can be used on things that are not model fields, in which caseNonewill be passed to as thefieldargument.class_validatorsdoes plenty of checks on the validator method signature before wrapping it, my implementation simply checks for the presence offield, and calls__modify_schema__()with or without thefieldparameter. This is because with validators there is separate prepare and execute steps, whereas__modify_schema__()is called directly when generating schema. If the signature doesn't match the expected, the error message and stack trace will be clear enough.Related issue number
There are multiple requests for this:
Checklist
changes/<pull request or issue id>-<github username>.mdfile added describing change(see changes/README.md for details)