8000 Make path of the item to validate available in plugin by hramezani · Pull Request #7861 · pydantic/pydantic · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@hramezani
Copy link
Member
@hramezani hramezani commented Oct 18, 2023

Change Summary

Related issue number

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

@cloudflare-workers-and-pages
Copy link
cloudflare-workers-and-pages bot commented Oct 18, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 17b7125
Status: ✅  Deploy successful!
Preview URL: https://b0f00146.pydantic-docs2.pages.dev
Branch Preview URL: https://plugin-path.pydantic-docs2.pages.dev

View logs

@hramezani hramezani force-pushed the plugin-path branch 2 times, most recently from a2c10ac to 77b401b Compare October 18, 2023 14:21
@hramezani
Copy link
Member Author

Please review

@adriangb
Copy link
Member

Should we be using __qualname__? Or otherwise, how are we handling e.g. a class nested in a function / differentiating them if there are multiple with the same name?

@adriangb
Copy link
Member

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 dict[cls, ??] which would be easier with actual classes instead of names.

@hramezani
8000 Copy link
Member Author
def foo():
    class Model(BaseModel): pass

def bar():
    class Model(BaseModel): pass

foo()
bar()

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 ?

Copy link
Member
@samuelcolvin samuelcolvin left a 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.

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Oct 19, 2023
@pydantic-hooky pydantic-hooky bot assigned hramezani and unassigned samuelcolvin Oct 19, 2023
@hramezani
Copy link
Member Author

@samuelcolvin I've addressed your comments.

Also, I've pushed a new commit and added item_type param that contains the type of the item to be validated. e.g BaseModel, create_model, dataclass, ...

please review

@pydantic-hooky pydantic-hooky bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Oct 20, 2023
@pydantic-hooky pydantic-hooky bot assigned sydney-runkle and unassigned hramezani Oct 20, 2023
@hramezani
Copy link
Member Author

I've added source_type as well.

Here are the params:

  • source_type: str -> The model, dataclass or type itself.
  • type_path: str -> It's path e.g. foo.bar.MyModel
  • item_type: str -> it's type e.g BaseModel, create_model, dataclass

TBH, I am not happy with param names. Any suggestions?

Copy link
Member
@samuelcolvin samuelcolvin left a 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.

Comment on lines 30 to 32
source_type: str,
type_path: str,
item_type: str,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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_type is a very confusing name, better to use schema_kind and make it a literal
  • I think it's more pythonic/correct ot make type_path, now schema_type_path an enum of module, name - you could even used a NamedTuple? Sorry for confusion.

source_type: Any,
module: str,
type_name: str,
item_type: str,
Copy link
Member

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.

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Oct 25, 2023
@pydantic-hooky pydantic-hooky bot assigned hramezani and unassigned sydney-runkle Oct 25, 2023
@pydantic-hooky pydantic-hooky bot assigned sydney-runkle and unassigned hramezani Oct 26, 2023
Copy link
Member
@samuelcolvin samuelcolvin left a 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,
Copy link
Member

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__?

Copy link
Member

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?

Copy link
Member Author

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

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Oct 30, 2023
@pydantic-hooky pydantic-hooky bot assigned hramezani and unassigned sydney-runkle Oct 30, 2023
@hramezani
Copy link
Member Author

Ok, @samuelcolvin I've addressed your comments

Please review again

@pydantic-hooky pydantic-hooky bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Oct 31, 2023
@pydantic-hooky pydantic-hooky bot assigned sydney-runkle and unassigned hramezani Oct 31, 2023
Copy link
Member
@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

0