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

wit/bindgen: Add JSON tag to Record structs #265

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

Conversation

lxfontes
Copy link
Member

@lxfontes lxfontes commented Dec 9, 2024

Related to #239

HostLayout receives json:"-" and any other field receives the original wit name.

// DateTime represents the record "wasi:clocks/[email protected]#datetime".
//
// A time and date in seconds plus nanoseconds.
//
//	record datetime {
//		seconds: u64,
//		nanoseconds: u32,
//	}
type DateTime struct {
	_           cm.HostLayout `json:"-"`
	Seconds     uint64        `json:"seconds"`
	Nanoseconds uint32        `json:"nanoseconds"`
}

@ydnar
Copy link
Collaborator

ydnar commented Dec 9, 2024

Don't forget to go generate ./tests/...

Signed-off-by: Lucas Fontes <[email protected]>
@ydnar
Copy link
Collaborator

ydnar commented Dec 10, 2024

How do we want to think about zero-value structs? Do we want to serialize, or have omitzero or omitempty?

@lxfontes
Copy link
Member Author

Ideally I'd like omitzero, however by adding it we complicate:

  • Tuples: encoded as lists. need to make sure we emit [ "field0", null, "field2"] and not ["field0","field2"]. Encoding it as dictionary would couple the json keys to cm.Tuple field names. I rather keep them as lists.
  • Enums/Variants: The zero value is valid. I feel this is by WIT design so the first value acts as a sentinel, but it is up to developers to actually use it that way.

feels like omitempty would be the least problematic, not raising the issues above.

For now I'm adding omitempty and we can revisit if needed after addressing Variant encoding.

@ydnar
Copy link
Collaborator

ydnar commented Dec 11, 2024

Given that Go omits fields tagged with omitempty that are the zero value for the type, this may not be what we want.

Maybe we start without omitempty and then add it in certain cases as an optimization?

@lxfontes lxfontes force-pushed the lxfontes/serialize-tags branch from 68397b3 to 17ff28c Compare December 11, 2024 01:17
@lxfontes
Copy link
Member Author

👍
removed omitempty.

@ydnar
Copy link
Collaborator

ydnar commented Dec 27, 2024

Looks ready to merge! Can you add a note to CHANGELOG.md?

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