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

Preserve semantic options in Encoder and Decoder #152

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Jan 29, 2025

A jsontext.Encoder and jsontext.Decoder could be constructed with semantic options (e.g., json.Deterministic) where such options are ignored for all encode/decode specific operations.

However, allow semantic options to take effect when such an Encoder or Decoder is passed to MarshalEncode or UnmarshalDecode. The semantic option can still be overridden.

One reason for this behavior is for easier migration. For example, this v1 code:

dec := jsonv1.NewDecoder(in)
dec.DisallowUnknownFields()
for {
	... := dec.Decode(...)
}

can be migrated as:

dec := jsontext.NewDecoder(in,
	json.RejectUnknownMembers(true))
for {
	... := json.UnmarshalDecode(dec, ...)
}

Notice that RejectUnknownMembers does not need
to be repeatedly passed to every UnmarshalDecode call because it is implicitly stored on the Decoder.

The alternative behavior is to have the construction of an Encoder or Decoder explicitly drop any semantic options. However, this seems like extra work for no benefit.

Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

I think this is reasonable and consistent.

@dsnet
Copy link
Collaborator Author

dsnet commented Feb 4, 2025

If we go with this change where we embrace the possibility of jsontext.Encoder or jsontext.Decoder to be able to store semantic options, then we should seriously consider @willfaught's suggestion of dropping an json.Options argument from the json.MarshalerTo and json.UnmarshalerFrom methods and instead expose an Options() json.Options method on Encoder and Decoder`.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

I like it

@johanbrandhorst
Copy link
Collaborator

If we go with this change where we embrace the possibility of jsontext.Encoder or jsontext.Decoder to be able to store semantic options, then we should seriously consider @willfaught's suggestion of dropping an json.Options argument from the json.MarshalerTo and json.UnmarshalerFrom methods and instead expose an Options() json.Options method on Encoder and Decoder`.

I like this too, if the options stick to the encoder/decoder, why have them as an extra argument? That seems like it would indeed just introduce confusion.

@dsnet dsnet force-pushed the preserve-semantic-options branch from 3750b1f to 04912c0 Compare February 12, 2025 22:49
A jsontext.Encoder and jsontext.Decoder could be constructed
with semantic options (e.g., json.Deterministic) where
such options are ignored for all encode/decode specific operations.

However, allow semantic options to take effect when such an
Encoder or Decoder is passed to MarshalEncode or UnmarshalDecode.
The semantic option can still be overridden.

One reason for this behavior is for easier migration.
For example, this v1 code:

	dec := jsonv1.NewDecoder(in)
	dec.DisallowUnknownFields()
	for {
		... := dec.Decode(...)
	}

can be migrated as:

	dec := jsontext.NewDecoder(in,
		json.RejectUnknownMembers(true))
	for {
		... := json.UnmarshalDecode(dec, ...)
	}

Notice that RejectUnknownMembers does not need
to be repeatedly passed to every UnmarshalDecode call
because it is implicitly stored on the Decoder.

The alternative behavior is to have the construction of
an Encoder or Decoder explicitly drop any semantic options.
However, this seems like extra work for no benefit.
@dsnet dsnet force-pushed the preserve-semantic-options branch from 04912c0 to 786819b Compare February 12, 2025 22:58
@dsnet dsnet merged commit 60a0516 into master Feb 12, 2025
8 checks passed
@dsnet dsnet deleted the preserve-semantic-options branch February 12, 2025 23:00
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.

3 participants