[go] encoding/json/v2: document custom marshal option adherence
Michael Pratt (Gerrit)
Michael Pratt voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Damien Neil
- Joseph Tsai
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Amsterdam (Gerrit)
Jonathan Amsterdam added 5 comments![]( "Open in Gerrit")
// Unless StringifyNumbers was applied globally, the option does notI'm confused by what it means to apply an option globally. I thought it meant at the top level of a marshal call, but the last sentence here contradicts that. So how do I set this option so that it applies recursively to numbers in arrays and objects?
// Determinism is guaranteed across different instances of the exact samenit: remove
or replace "exact same" with "identical"
// serialize in a deterministic manner if it is true.deterministically (?)
// serialize in a deterministic manner if it is true.a custom
// serialize in a deterministic manner if it is true.Maybe something like this:
There is only one nondeterministic operation typically encountered while traversing go data structures: iterating over a map produces the entries in random order. Sorting the keys is the usual fix, and that is in fact what the implementation does.
Related details
- Damien Neil
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Amsterdam (Gerrit)
Jonathan Amsterdam added 1 comment![]( "Open in Gerrit")
// serialize in a deterministic manner if it is true.Maybe something like this:
There is only one nondeterministic operation typically encountered while traversing go data structures: iterating over a map produces the entries in random order. Sorting the keys is the usual fix, and that is in fact what the implementation does.
or if you don't like "fix", then maybe "approach to making this deterministic"
Michael Pratt (Gerrit)
Michael Pratt uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Neil
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Neil
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt voted and added 5 comments![]( "Open in Gerrit")
Votes added by Michael Pratt
| Commit-Queue | +1 |
5 comments
// Unless StringifyNumbers was applied globally, the option does notI'm confused by what it means to apply an option globally. I thought it meant at the top level of a marshal call, but the last sentence here contradicts that. So how do I set this option so that it applies recursively to numbers in arrays and objects?
This option is a bit special (I hate this option).
The primary way to use this option is to set the "string" field tag. That is a "non-global" use: a field `f Foo "json:,string"` will see `StringifyNumbers` set in its MarshalJSONTo. But if that MarshalJSONTo calls MarshalEncode to marshal some sub-field then the option will be cleared.
A "global" use is simply passing `json.StringifyNumbers(true)` to a json.Marshal call, or any other `Marshal*` call.
This "global" term isn't well defined. I've rewritten this to just talk about the "string" tag vs explicit uses of the option. PTAL
// Determinism is guaranteed across different instances of the exact samenit: remove
or replace "exact same" with "identical"
Done
// serialize in a deterministic manner if it is true.Jonathan AmsterdamMaybe something like this:
There is only one nondeterministic operation typically encountered while traversing go data structures: iterating over a map produces the entries in random order. Sorting the keys is the usual fix, and that is in fact what the implementation does.
or if you don't like "fix", then maybe "approach to making this deterministic"
My new patch set on CL 809760 adds "For example, Go maps are marshaled sorted by key."
// serialize in a deterministic manner if it is true.Michael Pratta custom
MarshalFunc/MarshalToFunc can override marshaling of built in types, so it doesn't technically need to be a custom type.
// serialize in a deterministic manner if it is true.Michael Prattdeterministically (?)
Done
Related details
- Damien Neil
- Jonathan Amsterdam
- Joseph Tsai
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Amsterdam (Gerrit)
Jonathan Amsterdam voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jonathan Amsterdam
| Code-Review | +2 |
1 comment
// Unless StringifyNumbers was applied globally, the option does notMichael PrattI'm confused by what it means to apply an option globally. I thought it meant at the top level of a marshal call, but the last sentence here contradicts that. So how do I set this option so that it applies recursively to numbers in arrays and objects?
This option is a bit special (I hate this option).
The primary way to use this option is to set the "string" field tag. That is a "non-global" use: a field `f Foo "json:,string"` will see `StringifyNumbers` set in its MarshalJSONTo. But if that MarshalJSONTo calls MarshalEncode to marshal some sub-field then the option will be cleared.
A "global" use is simply passing `json.StringifyNumbers(true)` to a json.Marshal call, or any other `Marshal*` call.
This "global" term isn't well defined. I've rewritten this to just talk about the "string" tag vs explicit uses of the option. PTAL
- Damien Neil
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joseph Tsai (Gerrit)
Joseph Tsai added 5 comments![]( "Open in Gerrit")
// [jsontext.(*Encoder).Options] and adjust behavior to respect the options asDoes [jsontext.Encoder.Options] not work?
IIUC, you can't declare a non-pointer method if the pointer method already exists right? so there should be no ambiguity.
// - [StringifyNumbers]Perhaps comment on how doing so is only necessary if the representation itself is a JSON number? For most other cases where you're implementing a high-order composite type, then recursively calling `json.MarshalEncode` will handle this for you.
// The following options typically only apply to native Go types. These will beI feel like everything past this point is too much detail. I find references v1 legacy options in the v2 API unfortunate.
// - [StringifyNumbers]Perhaps comment on how doing so is only necessary if the representation itself is a JSON number? For most other cases where you're implementing a high-order composite type, then recursively calling json.MarshalEncode will handle this for you.
// The following options typically only apply to native Go types. These will beI feel like everything past this point is too much detail. I find references v1 legacy options in the v2 API unfortunate.
Related details
- Damien Neil
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +2 by Jonathan Amsterdam
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Neil
- Jonathan Amsterdam
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt added 6 comments![]( "Open in Gerrit")
// Implementations must not return [errors.ErrUnsupported].Are Marshaler implementations allowed to return errors.ErrUnsupported? They don't say one way or another.
// [jsontext.(*Encoder).Options] and adjust behavior to respect the options asDoes [jsontext.Encoder.Options] not work?
IIUC, you can't declare a non-pointer method if the pointer method already exists right? so there should be no ambiguity.
Done.
Perhaps comment on how doing so is only necessary if the representation itself is a JSON number? For most other cases where you're implementing a high-order composite type, then recursively calling `json.MarshalEncode` will handle this for you.
Done, though I'm not convinced what I've captured in one sentence is better than encouraging users to read the full docs.
// The following options typically only apply to native Go types. These will beI feel like everything past this point is too much detail. I find references v1 legacy options in the v2 API unfortunate.
I had two reasons for breaking out this first section:
1. To tell users to use MarshalEncode to marshal other types inside a composite (rather than e.g., attempting to manually encode a slice), so these options are respected.
2. If a custom type semantically represents a slice, they may want to check FormatNilSliceAsNull and format as "null" or "[]" for maximum compatibility with v1.
In retrospect, I'm not sure (2) is wise. Today, the type's MarshalJSON presumably returns "null". Users may migrate to v2 before you get a chance to add MarshalJSONTo, so they would still "null" in v2. When you do add MarshalJSONTo, they would see a change to "[]" without adjusting any options. That implies that custom types should use an alternative option for this kind of migration.
This could make more sense for a new custom type after 1.27, so there is no compatibility issue. But for a brand new type, there is also less reason why they would actually want "null".
(Or if you are implementing a MarshalToFunc for an actual slice type.)
I find references v1 legacy options in the v2 API unfortunate.
While I agree it is unfortunate, implementations of MarshalJSONTo need to be compatible with v1 users. If we need to reference v1 so they have the information they need to write a compatible implementation, so be it.
I've removed this list in favor of listing a single example option, but I still think an exhaustive list would be more helpful.
The final section is fine to drop.
Perhaps comment on how doing so is only necessary if the representation itself is a JSON number? For most other cases where you're implementing a high-order composite type, then recursively calling json.MarshalEncode will handle this for you.
Done
// The following options typically only apply to native Go types. These will beI feel like everything past this point is too much detail. I find references v1 legacy options in the v2 API unfortunate.
- Damien Neil
- Jonathan Amsterdam
- Joseph Tsai
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 1 comment![]( "Open in Gerrit")
// Implementations must not return [errors.ErrUnsupported].Are Marshaler implementations allowed to return errors.ErrUnsupported? They don't say one way or another.
Empirically, it appears that they are not. There's a check in makeMethodArshaler which converts ErrUnsupported into a "may not return errors.ErrUnsupported" error.
Related details
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt uploaded new patchset![]( "Open in Gerrit")
Related details
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Pratt
| Commit-Queue | +1 |
1 comment
// Implementations must not return [errors.ErrUnsupported].Damien NeilAre Marshaler implementations allowed to return errors.ErrUnsupported? They don't say one way or another.
Empirically, it appears that they are not. There's a check in makeMethodArshaler which converts ErrUnsupported into a "may not return errors.ErrUnsupported" error.
- Damien Neil
- Joseph Tsai
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joseph Tsai (Gerrit)
Joseph Tsai added 8 comments![]( "Open in Gerrit")
// Implementations must not retain or the value of T.Drop "or"
// Implementations must not retain or the value of T.Drop "or"
// types. [WithMarshalers] applies only to types other than the current type.I'm not sure I understand what this sentence means.
There's a blank line here that would break docs. Was this intentional?
// All other options are automatically handled outside of the MarshalerTo"All" seems a bit strong. Should we instead say "Most"?
// types. [WithUnmarshalers] applies only to types other than the current type.I'm not sure I understand what this sentence means.
There's a blank line here that would break docs. Was this intentional?
// All other options are automatically handled outside of the UnmarshalerFrom"All" seems a bit strong. Should we instead say "Most"?
Related details
- Damien Neil
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Neil
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt voted and added 8 comments![]( "Open in Gerrit")
Votes added by Michael Pratt
| Commit-Queue | +1 |
8 comments
// Implementations must not retain or the value of T.Michael PrattDrop "or"
Done
// Implementations must not retain or the value of T.Michael PrattDrop "or"
Done
// types. [WithMarshalers] applies only to types other than the current type.I'm not sure I understand what this sentence means.
A bunch of options, like FormatNilSliceAsNull only apply to native Go types, so they are not relevant to MarshalerTo implementations [1].
WithMarshalers applies to any Go type, so the previous sentence doesn't apply. But by definition, if MarshalJSONTo was called then WithMarshalers must not include a marshaler for this type (or it returned ErrUnsupported). Therefore WithMarshalers shouldn't matter to implementations, beyond being a motivation for calling MarshalEncode.
I agree that this wording is really confusing. I've attempted to reword, but I'm still not thrilled with the wording. PTAL.
[1] Well, if the type is a named slice, then maybe FormatNilSliceAsNull is relevant. Part of why I'm not a big fan of simplifying the docs.
There's a blank line here that would break docs. Was this intentional?
Good catch, thanks!
// All other options are automatically handled outside of the MarshalerTo"All" seems a bit strong. Should we instead say "Most"?
I'm not sure I agree. If there are other options relevant to MarshalJSONTo implementations then they should be listed here. That's the point of this CL.
"All other options" here means options not covered by the previous two paragraphs. But the previous paragraph no longer contains an exhaustive list, so it is certainly still vague.
// types. [WithUnmarshalers] applies only to types other than the current type.I'm not sure I understand what this sentence means.
Acknowledged
There's a blank line here that would break docs. Was this intentional?
Done
// All other options are automatically handled outside of the UnmarshalerFrom"All" seems a bit strong. Should we instead say "Most"?
- Damien Neil
- Joseph Tsai
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted Code-Review+1![]( "Open in Gerrit")
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |