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

msgspec: support omit_defaults option #2205

Open
ncoghlan opened this issue Dec 3, 2024 · 3 comments
Open

msgspec: support omit_defaults option #2205

ncoghlan opened this issue Dec 3, 2024 · 3 comments

Comments

@ncoghlan
Copy link
Contributor

ncoghlan commented Dec 3, 2024

Is your feature request related to a problem? Please describe.

When #1551 added msgspec support, it was noted that the omit_defaults option wasn't supported.

I'm currently choosing between Pydantic v2 and msgspec as my wire validation format, but need to be able to omit optional fields (the server complains if optional fields are populated on message types they don't apply to).

In Pydantic, whether to omit optional fields is set when dumping the model as a dict, in MsgSpec it needs to be set on the model classes themselves.

Describe the solution you'd like

An --omit-defaults option to set the option in the generated MsgSpec code.

Describe alternatives you've considered

Adding a custom template to include the option in the generated class entries.

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Dec 3, 2024

Looking at the default template, this may already be possible via the base_class_kwargs option in the extra_template_data arg, so this may actually be a request for improved extra_template_data documentation.

Attempting to infer the behaviour from code inspection:

  • the extra_template_data arg to generate is a string-keyed dict of string keyed dicts
  • the --extra-template-data CLI option is a string path to a JSON file containing the extra template data
  • the default templates already support a base_class_kwargs subdict for cases where that is useful

Assuming I've understood the behaviour, this feature already exists by way of passing extra_template_data = {"base_class_kwargs": {"omit_defaults": True}} when calling generate.

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Dec 3, 2024

I realised I was still missing something, as after attempting to set extra_template_data had no apparent affect, I inserted a definition into the generate call that should cause processing to fail outright:

        extra_template_data = defaultdict(
            lambda: {
                "base_class_kwargs": {
                    "omit_defaults": True,
                    "check_execution": 1/0
                }
            }
        )

(I also checked the version, and 0.26.3 is new enough that the class kwarg support should be in place for msgspec)

I then switched over to the following and it worked:

        extra_template_data = defaultdict(
            dict,
            {
                "#all#": {
                    "base_class_kwargs": {
                        "omit_defaults": True,
                    }
                }
            }
        )

This suggests to me that something somewhere is checking if extra_template_data: instead of if extra_template_data is not None, as the defaultdict-with-lambda solution only passes the second check, while using the #all# key passes both of them.

@ncoghlan
Copy link
Contributor Author

After resolving the extra_template_data issue I reported in #2215, I no longer think adding this feature is a good idea (the last #all# example shown above covers everything needed).

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

No branches or pull requests

1 participant