-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Custom base class detection #30
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
Thanks for the PR @sneakers-the-rat 🚀 However there's a simpler solution: instead of iterating on plugins:
- mkdocstrings:
handlers:
python:
options:
preload_modules:
- sqlmodel |
Oh wait, that's what we already do, actually. def _inherits_pydantic(cls: Class) -> bool:
for base in cls.bases:
if isinstance(base, (ExprName, Expr)):
base = base.canonical_path # noqa: PLW2901
if base in {"pydantic.BaseModel", "pydantic.main.BaseModel"}:
return True
return any(_inherits_pydantic(parent_class) for parent_class in cls.mro()) So we just need to update the documentation IMO. Could you try preloading |
Ah, ill give that a shot. I figured there was a difference between static and dynamic modes, where we would need to make the bases statically available in some contexts and dynamically in others, but now that I'm looking again it seems like we will always be importing anyway. Oh well still a good learning experience, hadn't tinkered around with a mkdocs plugin before, very tidy system compared to sphinx. If that works I'll close this and open another with a docs update |
ok good news and bad news
So it seems like we'll need this PR plus a few more to be able to properly handle both of them. Maybe for this PR i can add some notes to the docs like "for most cases, preloading the module should work to get it to be recognized, but for some others that manipulate model creation, you need to explicitly declare the base classes like..." Then i can follow on with another for excluding the dunder methods from the |
https://github.com/fastapi/sqlmodel/blob/main/sqlmodel/main.py#L770
There is indeed a difference between static and dynamic analysis. Static analysis doesn't import anything and only infers inheritance from the sources, while dynamic analysis does import classes to inspect their bases.
Thanks! Worth noting though is that we're solely using the Griffe extension system here, which knows nothing about MkDocs and its plugin system 🙂 flowchart TD
mkd["MkDocs"]
mkds["mkdocstrings (MkDocs plugin)"]
mkdsp["mkdocstrings-python (mkdocstrings handler)"]
g["Griffe (API data extractor for Python)"]
gp["griffe-pydantic (Griffe extension)"]
mkd --> mkds
mkds --> mkdsp
mkdsp --> g
g --> gp
Lets try to stay focused on one item at once. This PR is about supporting pydantic-settings, solving #27, right? So lets try to achieve that without concerning ourselves with SQLModel yet. You reported another issue with dunder fields: could you please open a new issue, if it makes sense? |
Good call. Ok I'll focus this on pydantic-settings, which I think just needs docs, open another issue, and then work on sqlmodel in another pr |
Fix: #27
Here's an attempt at #27. If the approach is OK, i'll write the docs for it :)
New to mkdocs, so sorry if i did this wrong.
The gist is rather than keeping an infinite list of all the possible things that could be pydantic models, we just allow someone to specify a non-standard base for detection of e.g. sqlmodel, pydantic-settings, and any other wrappers. hopefully this saves us from having to import them (?)
Two ways:
Adding to the defaults
Replacing the defaults (if we for some reason didn't want to document regular pydantic models and only wanted to document our special kind
edit: need to fix that docs build check but i'm not exactly sure what the configuration error is from the message. sorry for the red-x-open, will get to this tmrw