Skip to content

Commit

Permalink
Adjust single-line formatting behavior
Browse files Browse the repository at this point in the history 
Changes made:

* Further separate the behavior of SpaceAfterColon and SpaceAfterComma
  from Multiline. That is, use of Multiline does not affect the behavior
  of SpaceAfterColon or SpaceAfterComma unless it has not been set.
  For example, specifying (SpaceAfterColon(false), Multiline(true))
  should avoid the space after the colon even if we expect this
  combination of options to be seldom used.

* Exclude whitespace formatting flags from DefaultOptionsV1
  since v1 has API support for both single-line and multi-line output,
  so setting (or clearing) them cannot be classified as v1 behavior.
  Similarly, exclude whitespace formatting flags from DefaultOptionsV2.

* Simplify the TestMarshal cases.

* Add explicit TestEncoder cases to exercise all
  Encoder.WriteValue code paths.
  • Loading branch information
@dsnet @mvdan
dsnet authored and mvdan committed Apr 12, 2024
1 parent b15d3ef commit 37be135
Show file tree
Hide file tree
Showing 10 changed files with 114 additions and 154 deletions.
5 changes: 2 additions & 3 deletions arshal_default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -975,10 +975,9 @@ func makeStructArshaler(t reflect.Type) *arshaler {
// Append any delimiters or optional whitespace.
b := xe.Buf
if xe.Tokens.Last.Length() > 0 {
b = append(b, ',')
if xe.Flags.Get(jsonflags.SpaceAfterComma) {
b = append(b, ',', ' ')
} else {
b = append(b, ',')
b = append(b, ' ')
}
}
if xe.Flags.Get(jsonflags.Multiline) {
Expand Down
90 changes: 10 additions & 80 deletions arshal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1233,89 +1233,19 @@ func TestMarshal(t *testing.T) {
}`,
}, {
name: jsontest.Name("Structs/SpaceAfterColonAndComma"),
opts: []Options{
jsontext.SpaceAfterColon(true),
jsontext.SpaceAfterComma(true),
},
in: structAll{
Bool: true,
String: "hello",
Bytes: []byte{1, 2, 3},
Int: -64,
Uint: +64,
Float: 3.14159,
Map: map[string]string{"key": "value"},
StructScalars: structScalars{
Bool: true,
String: "hello",
Bytes: []byte{1, 2, 3},
Int: -64,
Uint: +64,
Float: 3.14159,
},
StructMaps: structMaps{
MapBool: map[string]bool{"": true},
MapString: map[string]string{"": "hello"},
MapBytes: map[string][]byte{"": {1, 2, 3}},
MapInt: map[string]int64{"": -64},
MapUint: map[string]uint64{"": +64},
MapFloat: map[string]float64{"": 3.14159},
},
StructSlices: structSlices{
SliceBool: []bool{true},
SliceString: []string{"hello"},
SliceBytes: [][]byte{{1, 2, 3}},
SliceInt: []int64{-64},
SliceUint: []uint64{+64},
SliceFloat: []float64{3.14159},
},
Slice: []string{"fizz", "buzz"},
Array: [1]string{"goodbye"},
Pointer: new(structAll),
Interface: (*structAll)(nil),
},
want: `{"Bool": true, "String": "hello", "Bytes": "AQID", "Int": -64, "Uint": 64, "Float": 3.14159, "Map": {"key": "value"}, "StructScalars": {"Bool": true, "String": "hello", "Bytes": "AQID", "Int": -64, "Uint": 64, "Float": 3.14159}, "StructMaps": {"MapBool": {"": true}, "MapString": {"": "hello"}, "MapBytes": {"": "AQID"}, "MapInt": {"": -64}, "MapUint": {"": 64}, "MapFloat": {"": 3.14159}}, "StructSlices": {"SliceBool": [true], "SliceString": ["hello"], "SliceBytes": ["AQID"], "SliceInt": [-64], "SliceUint": [64], "SliceFloat": [3.14159]}, "Slice": ["fizz", "buzz"], "Array": ["goodbye"], "Pointer": {"Bool": false, "String": "", "Bytes": "", "Int": 0, "Uint": 0, "Float": 0, "Map": {}, "StructScalars": {"Bool": false, "String": "", "Bytes": "", "Int": 0, "Uint": 0, "Float": 0}, "StructMaps": {"MapBool": {}, "MapString": {}, "MapBytes": {}, "MapInt": {}, "MapUint": {}, "MapFloat": {}}, "StructSlices": {"SliceBool": [], "SliceString": [], "SliceBytes": [], "SliceInt": [], "SliceUint": [], "SliceFloat": []}, "Slice": [], "Array": [""], "Pointer": null, "Interface": null}, "Interface": null}`,
opts: []Options{jsontext.SpaceAfterColon(true), jsontext.SpaceAfterComma(true)},
in: structOmitZeroAll{Int: 1, Uint: 1},
want: `{"Int": 1, "Uint": 1}`,
}, {
name: jsontest.Name("Structs/SpaceAfterColon"),
opts: []Options{jsontext.SpaceAfterColon(true)},
in: structOmitZeroAll{Int: 1, Uint: 1},
want: `{"Int": 1,"Uint": 1}`,
}, {
name: jsontest.Name("Structs/SpaceAfterComma"),
opts: []Options{jsontext.SpaceAfterComma(true)},
in: structAll{
Bool: true,
String: "hello",
Bytes: []byte{1, 2, 3},
Int: -64,
Uint: +64,
Float: 3.14159,
Map: map[string]string{"key": "value"},
StructScalars: structScalars{
Bool: true,
String: "hello",
Bytes: []byte{1, 2, 3},
Int: -64,
Uint: +64,
Float: 3.14159,
},
StructMaps: structMaps{
MapBool: map[string]bool{"": true},
MapString: map[string]string{"": "hello"},
MapBytes: map[string][]byte{"": {1, 2, 3}},
MapInt: map[string]int64{"": -64},
MapUint: map[string]uint64{"": +64},
MapFloat: map[string]float64{"": 3.14159},
},
StructSlices: structSlices{
SliceBool: []bool{true},
SliceString: []string{"hello"},
SliceBytes: [][]byte{{1, 2, 3}},
SliceInt: []int64{-64},
SliceUint: []uint64{+64},
SliceFloat: []float64{3.14159},
},
Slice: []string{"fizz", "buzz"},
Array: [1]string{"goodbye"},
Pointer: new(structAll),
Interface: (*structAll)(nil),
},
want: `{"Bool":true, "String":"hello", "Bytes":"AQID", "Int":-64, "Uint":64, "Float":3.14159, "Map":{"key":"value"}, "StructScalars":{"Bool":true, "String":"hello", "Bytes":"AQID", "Int":-64, "Uint":64, "Float":3.14159}, "StructMaps":{"MapBool":{"":true}, "MapString":{"":"hello"}, "MapBytes":{"":"AQID"}, "MapInt":{"":-64}, "MapUint":{"":64}, "MapFloat":{"":3.14159}}, "StructSlices":{"SliceBool":[true], "SliceString":["hello"], "SliceBytes":["AQID"], "SliceInt":[-64], "SliceUint":[64], "SliceFloat":[3.14159]}, "Slice":["fizz", "buzz"], "Array":["goodbye"], "Pointer":{"Bool":false, "String":"", "Bytes":"", "Int":0, "Uint":0, "Float":0, "Map":{}, "StructScalars":{"Bool":false, "String":"", "Bytes":"", "Int":0, "Uint":0, "Float":0}, "StructMaps":{"MapBool":{}, "MapString":{}, "MapBytes":{}, "MapInt":{}, "MapUint":{}, "MapFloat":{}}, "StructSlices":{"SliceBool":[], "SliceString":[], "SliceBytes":[], "SliceInt":[], "SliceUint":[], "SliceFloat":[]}, "Slice":[], "Array":[""], "Pointer":null, "Interface":null}, "Interface":null}`,
in: structOmitZeroAll{Int: 1, Uint: 1, Slice: []string{"a", "b"}},
want: `{"Int":1, "Uint":1, "Slice":["a", "b"]}`,
}, {
name: jsontest.Name("Structs/Stringified"),
opts: []Options{jsontext.Multiline(true)},
Expand Down
5 changes: 5 additions & 0 deletions internal/jsonflags/flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ const (

// AnyWhitespace reports whether the encoded output might have any whitespace.
AnyWhitespace = Multiline | SpaceAfterColon | SpaceAfterComma

// WhitespaceFlags is the set of flags related to whitespace formatting.
// In contrast to AnyWhitespace, this includes Indent and IndentPrefix
// as those settings take no effect if Multiline is false.
WhitespaceFlags = AnyWhitespace | Indent | IndentPrefix
)

// Encoder and decoder flags.
Expand Down
16 changes: 2 additions & 14 deletions internal/jsonopts/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,17 @@ type ArshalValues struct {
// DefaultOptionsV2 is the set of all options that define default v2 behavior.
var DefaultOptionsV2 = Struct{
Flags: jsonflags.Flags{
Presence: uint64(jsonflags.AllFlags),
Presence: uint64(jsonflags.AllFlags & ^jsonflags.WhitespaceFlags),
Values: uint64(0),
},
CoderValues: CoderValues{Indent: "\t"}, // Indent is set, but Multiline is set to false
}

// DefaultOptionsV1 is the set of all options that define default v1 behavior.
var DefaultOptionsV1 = Struct{
Flags: jsonflags.Flags{
Presence: uint64(jsonflags.AllFlags),
Presence: uint64(jsonflags.AllFlags & ^jsonflags.WhitespaceFlags),
Values: uint64(jsonflags.DefaultV1Flags),
},
CoderValues: CoderValues{Indent: "\t"}, // Indent is set, but Multiline is set to false
}

// CopyCoderOptions copies coder-specific options from src to dst.
Expand Down Expand Up @@ -130,22 +128,12 @@ func (dst *Struct) Join(srcs ...Options) {
case nil:
continue
case jsonflags.Bools:
switch src {
case jsonflags.Multiline | 1:
dst.Flags.Clear(jsonflags.SpaceAfterComma | jsonflags.SpaceAfterColon)
case jsonflags.SpaceAfterComma | 1, jsonflags.SpaceAfterColon | 1:
if dst.Flags.Get(jsonflags.Multiline) {
continue
}
}
dst.Flags.Set(src)
case Indent:
dst.Flags.Set(jsonflags.Multiline | jsonflags.Indent | 1)
dst.Flags.Clear(jsonflags.SpaceAfterComma | jsonflags.SpaceAfterColon)
dst.Indent = string(src)
case IndentPrefix:
dst.Flags.Set(jsonflags.Multiline | jsonflags.IndentPrefix | 1)
dst.Flags.Clear(jsonflags.SpaceAfterComma | jsonflags.SpaceAfterColon)
dst.IndentPrefix = string(src)
case ByteLimit:
dst.Flags.Set(jsonflags.ByteLimit | 1)
Expand Down
18 changes: 15 additions & 3 deletions internal/jsonopts/options_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,22 @@ func TestJoin(t *testing.T) {
CoderValues: CoderValues{Indent: "\t"},
},
}, {
in: &DefaultOptionsV1, want: &DefaultOptionsV1, // v1 fully replaces before
in: &DefaultOptionsV1, want: func() *Struct {
v1 := DefaultOptionsV1
v1.Flags.Set(jsonflags.Indent | 1)
v1.Flags.Set(jsonflags.Multiline | 0)
v1.Indent = "\t"
return &v1
}(), // v1 fully replaces before (except for whitespace related flags)
}, {
in: &DefaultOptionsV2, want: &DefaultOptionsV2}, // v2 fully replaces before
}
in: &DefaultOptionsV2, want: func() *Struct {
v2 := DefaultOptionsV2
v2.Flags.Set(jsonflags.Indent | 1)
v2.Flags.Set(jsonflags.Multiline | 0)
v2.Indent = "\t"
return &v2
}(), // v2 fully replaces before (except for whitespace related flags)
}}
got := new(Struct)
for i, tt := range tests {
got.Join(tt.in)
Expand Down
38 changes: 22 additions & 16 deletions jsontext/encode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,17 @@ func (e *encoderState) reset(b []byte, w io.Writer, opts ...Options) {
}
e.Struct = jsonopts.Struct{}
e.Struct.Join(opts...)
if e.Flags.Get(jsonflags.Multiline) && !e.Flags.Has(jsonflags.Indent) {
e.Indent = "\t"
if e.Flags.Get(jsonflags.Multiline) {
if !e.Flags.Has(jsonflags.SpaceAfterColon) {
e.Flags.Set(jsonflags.SpaceAfterColon | 1)
}
if !e.Flags.Has(jsonflags.SpaceAfterComma) {
e.Flags.Set(jsonflags.SpaceAfterComma | 0)
}
if !e.Flags.Has(jsonflags.Indent) {
e.Flags.Set(jsonflags.Indent | 1)
e.Indent = "\t"
}
}
}

Expand Down Expand Up @@ -274,7 +283,6 @@ func (e *encoderState) UnwriteEmptyObjectMember(prevName *string) bool {
b = b[:len(b)-n]
b = jsonwire.TrimSuffixWhitespace(b)
b = jsonwire.TrimSuffixByte(b, ':')
b = jsonwire.TrimSuffixWhitespace(b)
b = jsonwire.TrimSuffixString(b)
b = jsonwire.TrimSuffixWhitespace(b)
b = jsonwire.TrimSuffixByte(b, ',')
Expand Down Expand Up @@ -582,15 +590,15 @@ func (e *encoderState) WriteValue(v Value) error {
// appendWhitespace appends whitespace that immediately precedes the next token.
func (e *encoderState) appendWhitespace(b []byte, next Kind) []byte {
if delim := e.Tokens.needDelim(next); delim == ':' {
if e.Flags.Get(jsonflags.Multiline | jsonflags.SpaceAfterColon) {
return append(b, ' ')
if e.Flags.Get(jsonflags.SpaceAfterColon) {
b = append(b, ' ')
}
} else {
if e.Flags.Get(jsonflags.Multiline) {
return e.AppendIndent(b, e.Tokens.NeedIndent(next))
}
if delim == ',' && e.Flags.Get(jsonflags.SpaceAfterComma) {
return append(b, ' ')
b = append(b, ' ')
}
if e.Flags.Get(jsonflags.Multiline) {
b = e.AppendIndent(b, e.Tokens.NeedIndent(next))
}
}
return b
Expand Down Expand Up @@ -726,7 +734,7 @@ func (e *encoderState) reformatObject(dst []byte, src Value, depth int) ([]byte,
}
dst = append(dst, ':')
n += len(":")
if e.Flags.Get(jsonflags.Multiline | jsonflags.SpaceAfterColon) {
if e.Flags.Get(jsonflags.SpaceAfterColon) {
dst = append(dst, ' ')
}

Expand All @@ -748,10 +756,9 @@ func (e *encoderState) reformatObject(dst []byte, src Value, depth int) ([]byte,
}
switch src[n] {
case ',':
dst = append(dst, ',')
if e.Flags.Get(jsonflags.SpaceAfterComma) {
dst = append(dst, ',', ' ')
} else {
dst = append(dst, ',')
dst = append(dst, ' ')
}
n += len(",")
continue
Expand Down Expand Up @@ -819,10 +826,9 @@ func (e *encoderState) reformatArray(dst []byte, src Value, depth int) ([]byte,
}
switch src[n] {
case ',':
dst = append(dst, ',')
if e.Flags.Get(jsonflags.SpaceAfterComma) {
dst = append(dst, ',', ' ')
} else {
dst = append(dst, ',')
dst = append(dst, ' ')
}
n += len(",")
continue
Expand Down
30 changes: 30 additions & 0 deletions jsontext/encode_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,36 @@ var encoderErrorTestdata = []struct {
{ArrayEnd, nil, ""},
},
wantOut: "[]\n",
}, {
name: jsontest.Name("Format/Object/SpaceAfterColon"),
opts: []Options{SpaceAfterColon(true)},
calls: []encoderMethodCall{{Value(`{"fizz":"buzz","wizz":"wuzz"}`), nil, ""}},
wantOut: "{\"fizz\": \"buzz\",\"wizz\": \"wuzz\"}\n",
}, {
name: jsontest.Name("Format/Object/SpaceAfterComma"),
opts: []Options{SpaceAfterComma(true)},
calls: []encoderMethodCall{{Value(`{"fizz":"buzz","wizz":"wuzz"}`), nil, ""}},
wantOut: "{\"fizz\":\"buzz\", \"wizz\":\"wuzz\"}\n",
}, {
name: jsontest.Name("Format/Object/SpaceAfterColonAndComma"),
opts: []Options{SpaceAfterColon(true), SpaceAfterComma(true)},
calls: []encoderMethodCall{{Value(`{"fizz":"buzz","wizz":"wuzz"}`), nil, ""}},
wantOut: "{\"fizz\": \"buzz\", \"wizz\": \"wuzz\"}\n",
}, {
name: jsontest.Name("Format/Object/NoSpaceAfterColon+SpaceAfterComma+Multiline"),
opts: []Options{SpaceAfterColon(false), SpaceAfterComma(true), Multiline(true)},
calls: []encoderMethodCall{{Value(`{"fizz":"buzz","wizz":"wuzz"}`), nil, ""}},
wantOut: "{\n\t\"fizz\":\"buzz\", \n\t\"wizz\":\"wuzz\"\n}\n",
}, {
name: jsontest.Name("Format/Array/SpaceAfterComma"),
opts: []Options{SpaceAfterComma(true)},
calls: []encoderMethodCall{{Value(`["fizz","buzz"]`), nil, ""}},
wantOut: "[\"fizz\", \"buzz\"]\n",
}, {
name: jsontest.Name("Format/Array/NoSpaceAfterComma+Multiline"),
opts: []Options{SpaceAfterComma(false), Multiline(true)},
calls: []encoderMethodCall{{Value(`["fizz","buzz"]`), nil, ""}},
wantOut: "[\n\t\"fizz\",\n\t\"buzz\"\n]\n",
}}

// TestEncoderErrors test that Encoder errors occur when we expect and
Expand Down
Loading

0 comments on commit 37be135

Please sign in to comment.