-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add support for programmatic title generation #9183
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #9183 will not alter performanceComparing Summary
|
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.
Hey @NeevCohen,
Great work, as usual!
I want to look again at _generate_schema.py
and do a more in-depth look at your tests (great work btw, looks very thorough). This looks very promising though - I've included some initial feedback / some minor change requests.
One other change I'd like to see is the addition of some documentation in the concepts
section of our docs!
Co-authored-by: Sydney Runkle <54324534+sydney-runkle@users.noreply.github.com>
…eature/title-generators
@sydney-runkle Thanks! I implemented the changes you requested. |
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.
Question : why not having title be a string or a callable that returns a string instead of two params?
This could be extended to alias generation for example with a context passed as arg in case of callable
@PrettyWood I implemented it the way the original issue suggested, I'm fine with changing it if we decide that is the way we want to go. |
@NeevCohen totally agree. Probably a change for v3 then |
@PrettyWood Awesome. |
Sorry for the delay, was out of the office last week. Looking now 🚀 ! Re your concepts docs question - your changes look great. We might want to add some more cross links with the config / fields sections, but it looks awesome thus far. |
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.
Great work here. Really impressed with the thorough testing, and great work on the docs as well.
Let's have @samuelcolvin do a final review of this given that he made the original feature request. Stay tuned! |
Apply suggestion Co-authored-by: Sydney Runkle <54324534+sydney-runkle@users.noreply.github.com>
Sorry for the delay here, we'll get this across the line for 2.8! |
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.
overall looks like a good idea, but there are a few things to iron out.
from pydantic import BaseModel, Field | ||
|
||
|
||
def make_title(field_name: str) -> str: |
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.
my main question is whether the full field should be passed to the function, not just the name.
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 full field as in the FieldInfo
object? That we need to pass both FieldInfo
and the name of the field since FieldInfo
doesn't include the name.
Then the signature of make_title
would be
def make_title(field_name: str, field_info: FieldInfo) -> str: ...
from pydantic import BaseModel, ConfigDict | ||
|
||
|
||
def make_title(field_name: str) -> str: |
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.
again I would suggest we pass the cls
itself, as well as the current name.
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.
Sure.
What type do you reckon the cls
parameter be annotated as? Type[BaseModel]
?
pydantic/config.py
Outdated
@@ -33,11 +33,18 @@ class ConfigDict(TypedDict, total=False): | |||
title: str | None | |||
"""The title for the generated JSON schema, defaults to the model's name""" | |||
|
|||
class_title_generator: Callable[[str], str] | None | |||
"""A callable that takes a class name and returns the title for it. Defaults to `None`.""" |
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 should be clear about whether this works for:
- pydantic models?
- dataclasses?
- typeddicts?
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 works for all model and model-like (pydantic dataclass, builtin dataclass, typeddict) types. ConfigDict
can be applied to all of them so IMO it's a little redundant. Changing the name to model_title_generator
makes this even more clear I feel.
I will add more documentation if you feel it's necessary.
@@ -106,6 +108,8 @@ class FieldInfo(_repr.Representation): | |||
validation_alias: The validation alias of the field. | |||
serialization_alias: The serialization alias of the field. | |||
title: The title of the field. | |||
title_priority: Priority of the field's title. This affects whether a title generator is used. |
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.
Are you sure we need this? If so, I think it needs more documentation.
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 guess I wanted to have a similar API to alias_generator
and alias_priority
. Do you think it's worth keeping? If so I'll add more documentation.
@NeevCoehn, Just a side note, we have a Thanks for all of your great work! |
@sydney-runkle Awesome! Will do! Thanks |
Change Summary
Implement programmatic title generation via
ConfigDict
andField
Related issue number
fix #4632
Checklist
Selected Reviewer: @dmontagu