-
-
Notifications
You must be signed in to change notification settings - Fork 203
Support defaults from Pydantic fields #802
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
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.
Thanks!
Should Pydantic support for pdoc be split out into a separate "plugin" package, e.g. pdoc-pydantic or pdoc[pydantic]?
Too much complexity for now. I think pydantic is popular enough to have support for it builtin for now.
Is this the best place for default-value-extraction logic for Pydantic classes to live?
Pragmatically yes. See my comments below on how we can keep things reasonably clean.
This comment was marked as resolved.
This comment was marked as resolved.
pdoc/doc.py
Outdated
# For Pydantic models, filter out all methods on the BaseModel | ||
# class, as they are almost never relevant to the consumers of the | ||
# inheriting model itself. | ||
if ( | ||
_pydantic._PYDANTIC_ENABLED | ||
and self.kind == "class" | ||
and _pydantic.is_pydantic_model(self.obj) | ||
and ( | ||
name in _pydantic._IGNORED_FIELDS | ||
or taken_from[0].startswith("pydantic") | ||
) | ||
): | ||
continue | ||
|
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.
@mhils: Let me know what you think of this approach. It kind of contradicts the precedent members()
sets of deferring all filtering/exclusion logic to the downstream templates.
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 think it's not the end of the world, but we could maybe move it into Class._member_objects
? That already has some special cases for constructors. Adding another special case for pydantic there makes more sense maybe.
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.
Thank you! This looks great overall. Mostly some minor stylistic nits.
pdoc/doc.py
Outdated
# For Pydantic models, filter out all methods on the BaseModel | ||
# class, as they are almost never relevant to the consumers of the | ||
# inheriting model itself. | ||
if ( | ||
_pydantic._PYDANTIC_ENABLED | ||
and self.kind == "class" | ||
and _pydantic.is_pydantic_model(self.obj) | ||
and ( | ||
name in _pydantic._IGNORED_FIELDS | ||
or taken_from[0].startswith("pydantic") | ||
) | ||
): | ||
continue | ||
|
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 think it's not the end of the world, but we could maybe move it into Class._member_objects
? That already has some special cases for constructors. Adding another special case for pydantic there makes more sense maybe.
test/testdata/with_pydantic.txt
Outdated
@@ -0,0 +1,7 @@ | |||
<module with_pydantic # A small example with… | |||
<class with_pydantic.Foo # Foo class documentat… | |||
<var a: int = 1> |
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.
This does not seem to be working yet? :)
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.
🤨 could swear this was working before.
Either way, will look into. Moving this back to draft.
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.
Oh, I'm trying to do two things at once. (:
This PR started off just handling Pydantic default values, but I've since expanded scope to also include descriptions.
Will revise PR title + description + all that before opening back up for review.
Co-authored-by: Maximilian Hils <[email protected]>
if ( | ||
_pydantic.pydantic is not None | ||
and self.kind == "class" | ||
and _pydantic.is_pydantic_model(self.obj) | ||
): | ||
_docstring = _pydantic.get_field_docstring(self.obj, 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.
Consolidate conditional logic into _pydantic
like w/ default_value()
.
Contributes to #793.
This PR implements quick-and-dirty support for Pydantic-style default values. In other words, the following two classes will render equivalently:
Open questions:
pdoc-pydantic
orpdoc[pydantic]
?