Skip to content
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

prototype: pydantic object type extension #2612

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

erikwrede
Copy link
Member

Prototype for #2605

@erikwrede erikwrede mentioned this pull request Mar 6, 2023
2 tasks
@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #2612 (96a4516) into main (3711ab5) will decrease coverage by 0.80%.
The diff coverage is 38.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2612      +/-   ##
==========================================
- Coverage   96.30%   95.50%   -0.80%     
==========================================
  Files         184      186       +2     
  Lines        7546     7661     +115     
  Branches     1385     1407      +22     
==========================================
+ Hits         7267     7317      +50     
- Misses        178      238      +60     
- Partials      101      106       +5     

@@ -193,6 +200,7 @@ def type(
is_interface: bool = False,
description: Optional[str] = None,
directives: Optional[Sequence[object]] = (),
extensions: Optional[List[TypeExtension]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

lame nitpick -> should at least follow directies and have a default of empty sequence. e.g.

directives: Optional[Sequence[object]] = (),
extensions: Optional[Sequence[TypeExtension]] = (),

though idk why we even needed the None case, it could just be

extensions: Sequence[TypeExtension]] = (),

@@ -46,6 +48,8 @@ class TypeDefinition(StrawberryType):
)

def __post_init__(self):
for extension in self.extensions:
extension.apply(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

cool stuff

@thejaminator
Copy link
Contributor

thejaminator commented Mar 7, 2023

looks great to me as a prototype for exploration.

I guess something to think about is how we'll retrieve some static information of the transformation. E.g. how do we get our static typecheckers to know that from_pydantic and to_pydantic exist now?
Related to discussions in #2168. I did try to play around to see how we could do that, but no strong ideas so far. so LGTM :)

assert field1.metadata["admin_only"]

assert field2.python_name == "public"
assert not field2.metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, i think you'll need some tests calling from pydantic or to_pydantic seems like PydanticTypeExtension doesn't define it too?

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, these were just the basic tests to see if the general idea works. Will add more soon 🙂
PydanticTypeExtension should also set the from and to methods, but type checking is indeed an issue

@erikwrede
Copy link
Member Author

erikwrede commented Mar 7, 2023

I guess something to think about is how we'll retrieve some static information of the transformation. E.g. how do we get our static typecheckers to know that from_pydantic and to_pydantic exist now?
Related to discussions in #2168. I did try to play around to see how we could do that, but no strong ideas so far. so LGTM :)

Good point. Maybe we could make all of these model dataclasses extend StrawberryTypeFromPydantic. That comes with the drawback of reducing explicitness, so I'm not yet sure if I like it or not. Though, it does only make a stylistic difference, not a functional difference.

  @strawberry.type(extensions=[PydanticTypeExtension(model=User)])
  class UserType(StrawberryTypeFromPydantic):
      age: strawberry.auto
      password: strawberry.auto

The advantage with this is that IDEs will automatically suggest from_pydantic and to_pydantic as options to override, so we gain explicitness on that front. It's a trade-off imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants