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

Support JSON tags for nullable enum structs #2121

Merged
merged 13 commits into from
Jun 9, 2023

Conversation

mikemherron
Copy link
Contributor

@mikemherron mikemherron commented Mar 1, 2023

Add JSON tags on to generated null enum structs. Resolves #2115.

However, while this works, it may be better implemented as a dedicated config option because:

  1. The current implementation only honours the emit_json_tags option. The emit_db_tags is ignored, as it doesn't really make sense in this context since the struct does not represent a database entity. I think this is a logical descision, but it may initially be confusing to users why the null enums have one type of tag but not the other.
  2. In the case of json_tags_case_style being "none", the tag name should fall back to the database column name. However, for the Valid property of the Null enum structs there is no corresponding database column, so "valid" will always be used which might be inconsistent with other fields.
  3. This could also be considered a breaking change as sqlc will now add JSON tags in to structs that did not have them previously (and would have been serialised/deserialised using default behaviour defined by Go).

An alternative to this (which I am happy to implement) would be to introduce a new config option, documented as so:

emit_json_tags_on_enum_structs - If true, and emit_json_tags is also true, add JSON tags on to generated null enum structs. Defaults to false. When true, the case style of the tags will follow that defined by json_tags_case_style, unless it is none, in which case the Valid property will use the tag format valid.

Which resolves the issues I raised. It feels a bit clunky, but I think it is clearer, and keeps the default behaviour as-is while supporting niche use-cases.

Opened the PR for feedback; I will wait for maintainer feedback before starting on the additional config option.

@mikemherron mikemherron changed the title Draft: Support JSON tags for nullable enum structs Support JSON tags for nullable enum structs Mar 2, 2023
@mikemherron mikemherron marked this pull request as ready for review March 2, 2023 13:39
@mikemherron
Copy link
Contributor Author

Sorry for the tag @kyleconroy but do you have an opinion on this one? If you are in agreement with my config proposal, I have time over the weekend to implement this and update the PR. If you'd rather leave this feature out entirely, no problem, feel free to close the issue.

Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

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

@mikemherron Apologies for not getting to this one sooner.

I think your last point is correct. This could be a breaking change for users, as the JSON format of nullable enums will change. Let's go with your second plan of adding a new configuration option to control this behavior.

@mikemherron
Copy link
Contributor Author

@mikemherron Apologies for not getting to this one sooner.

I think your last point is correct. This could be a breaking change for users, as the JSON format of nullable enums will change. Let's go with your second plan of adding a new configuration option to control this behavior.

No problem @kyleconroy, I should have time to do this over the next week.

QueryParameterLimit: s.QueryParameterLimit,
EmitInterface: s.EmitInterface,
EmitJsonTags: s.EmitJSONTags,
EmitJsonTagsOnNullEnumStructs: s.EmitJSONTagsOnNullEnumStructs,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, EmitJsonTagsOnNullEnumStructs is the longest config property name so causes go fmt to realign all the config structs, making the PR look a bit busier than it should in places like this.

@mikemherron mikemherron requested a review from kyleconroy April 26, 2023 13:28
@mikemherron
Copy link
Contributor Author

JSON tags on null enum structs are now off by defaullt, and must be turned on with the emit_json_tags_on_enum_structs config option. PR is ready to review again.

@kyleconroy
Copy link
Collaborator

After thinking about it a bit more, I decided that the extra configuration option isn't worth it. If emit_json_tags is on, we'll add the tags to nullable enums. I cleaned up the code slightly as well.

@kyleconroy kyleconroy merged commit dc85b37 into sqlc-dev:main Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null Enums Don't Emit JSON Tag
2 participants