-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
Support msgspec output #1551
Support msgspec output #1551
Conversation
Forces on use_annotated as that is mechanism that msgspec uses for contraints with msgspec.Meta
not required for generation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1551 +/- ##
===========================================
Coverage 100.00% 100.00%
===========================================
Files 11 34 +23
Lines 1020 3727 +2707
Branches 201 873 +672
===========================================
+ Hits 1020 3727 +2707
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@@ -283,6 +283,9 @@ def merge_args(self, args: Namespace) -> None: | |||
if getattr(args, f) is not None | |||
} | |||
|
|||
if set_args.get('output_model_type') == DataModelType.MsgspecStruct.value: | |||
set_args['use_annotated'] = True |
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.
@indrat
Doesn't msgspec use Field
? and is it always annotated?
You define some logic for Field
. But, the unit tests don't cover the parts.
We should change the coverage to 100% before merging.
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've added some output tests for when a msgspec.field
is generated and tried to boost the coverage for the const/alias fields as well.
Regarding forcinguse_annotated
I believe the constraints such as gt
, lt
, etc must be attached using Annotated[..., Meta(gt=..., lt=...)]
etc. So aiming for the least surprise I felt it made sense to always force use_annotated
on otherwise you won't get the constraints or description metadata generated.
msgspec.field
only accepts default
,default_factory
, and name
so it's a little different to pydantic.Field
@indrat The unittests don't cover all lines. |
nb. this adjusts the output for the pydantic test for Nested.Foo since the definition for Nested.Foo is a str not an object.
…_factory avoids issue with default values and mutuable lists
Thank you very much!! |
Adds basic support for
msgspec.Struct
as an output format which possibly addresses #1278I have not used this in anger yet, only to transform some of the examples so I cannot say whether it is completely correct but I figured it was a good idea to push it up for review. I basically followed the bouncing ball of changes from the previous two PRs to add
dataclass
andTypedDict
support.Things that I haven't figured out:
Class args
Class args to Struct for things like
kw_only
,forbid_unknown_fields
, oromit_defaults
, i.ethis is probably relevant for the dataclasses output as well for passing args to the
@dataclass
decoratordiscriminated unions
Somewhat relatedly - discriminated unions. msgspec uses class args,
tag
andtag_field
, to denote when Structs participate in a tagged Union whereas the pydantic output includes that information on the Field. Not sure how to solve this one. Ideally output similar to the ClassVar approach in jcrist/msgspec#338 (comment)pydantic output
with this branch msgspec output has no way to add the
tag
ortag_field
class args.Possible approach using
ClassVar
from jcrist/msgspec#338 (comment) requires being able to add class args liketag
/tag_field
.