-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Make path of the item to validate available in plugin #7861
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
Deploying with
|
| Latest commit: |
17b7125
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b0f00146.pydantic-docs2.pages.dev |
| Branch Preview URL: | https://plugin-path.pydantic-docs2.pages.dev |
a2c10ac to
77b401b
Compare
|
Please review |
|
Should we be using |
|
Can you add a test for that scenario? Something like: def foo():
class Model(BaseModel): pass
def bar():
class Model(BaseModel): pass
foo()
bar()Alternatively, instead of making the class name and module available why can't we just make the class itself available? Then the plugin can decide what to do with weird cases like above. Or plugins may want to keep a some soft of |
I've added a test for this. hmm, this is also an idea to make the class itself available on plugin. The current solution suggested by @samuelcolvin. So, what do you think here @samuelcolvin ? |
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.
please update.
4edb244 to
559734b
Compare
|
@samuelcolvin I've addressed your comments. Also, I've pushed a new commit and added please review |
559734b to
62ec577
Compare
|
I've added Here are the params:
TBH, I am not happy with param names. Any suggestions? |
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.
Sorry, some more changes to make it clearer. Hope this is makes sense to you.
pydantic/plugin/__init__.py
Outdated
| source_type: str, | ||
| type_path: str, | ||
| item_type: 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.
| source_type: str, | |
| type_path: str, | |
| item_type: str, | |
| schema_type: Any, | |
| schema_type_path: tuple[str, str], | |
| schema_kind: Literal['BaseModel', 'create_mode', 'validate_call', 'TypeAdapter'], |
Maybe I'm confused, but source_type should be Any I think?
Also:
item_typeis a very confusing name, better to useschema_kindand make it a literal- I think it's more pythonic/correct ot make
type_path, nowschema_type_pathan enum ofmodule,name- you could even used aNamedTuple? Sorry for confusion.
pydantic/plugin/_schema_validator.py
Outdated
| source_type: Any, | ||
| module: str, | ||
| type_name: str, | ||
| item_type: 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.
rename and change type types here to. You can leave schema_type_module and schema_type_name as separate parameters if you like.
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.
please update.
| namespace: dict[str, Any], | ||
| __pydantic_generic_metadata__: PydanticGenericMetadata | None = None, | ||
| __pydantic_reset_parent_namespace__: bool = True, | ||
| cls_module: str | None = 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.
Are you sure it's valid to pass cls_module as a kwarg to __new__?
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 module has to be passed this way for create_model, maybe better to use _cls_module to avoid clashes with config?
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.
Yes, it's there for create_model. I renamed all of them to _cls_module
e6b0504 to
218b61b
Compare
|
Ok, @samuelcolvin I've addressed your comments Please review 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
Change Summary
Related issue number
Checklist
Selected Reviewer: @sydney-runkle