Skip to content

Commit 8d1bedc

Browse files
benjaminjkraftbenjaminjkraft
authored
Put comments behind an option in formatter (#271)
* Put comments behind an option in formatter In #263, we introduced parsing of comments, as well as support for them in the formatter. In some cases this is surely useful, but in others it's just bloat. (And as I describe in Khan/genqlient#282, it may even be a problem in some cases which depended on the fact that formatting the query didn't include comments.) In this commit I introduce an option to control whether comments are formatted. I set the default to false (i.e. restoring the previous behavior if no options are set), because adding this felt to me like a breaking change, and because it seems to me like the more common usage. `WithComments()` restores the behavior added in #263. If others disagree I'm happy to keep the changed default, and instead provide `WithoutComments()`. I also added tests both ways (and for the existing `WithIndent()` option), and checked that the `comments` tests match the existing ones, and the `default` tests match those from `v2.6.3` (except for the addition of a few descriptions whose omission seem to have been a bug). Comments are still parsed in any case, as adding new struct fields is no problem; and they are still included in `Dump` since that seems obviously parallel to struct fields and is more of a debugging thing. * docs
1 parent 9fb1c32 commit 8d1bedc

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

68 files changed

+1018
-100
lines changed

‎formatter/formatter.go

+15-5
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,21 @@ type Formatter interface {
1818
//nolint:revive // Ignore "stuttering" name format.FormatterOption
1919
type FormatterOption func(*formatter)
2020

21+
// WithIndent uses the given string for indenting block bodies in the output,
22+
// instead of the default, `"\t"`.
2123
func WithIndent(indent string) FormatterOption {
2224
return func(f *formatter) {
2325
f.indent = indent
2426
}
2527
}
2628

29+
// WithComments includes comments from the source/AST in the formatted output.
30+
func WithComments() FormatterOption {
31+
return func(f *formatter) {
32+
f.emitComments = true
33+
}
34+
}
35+
2736
func NewFormatter(w io.Writer, options ...FormatterOption) Formatter {
2837
f := &formatter{
2938
indent: "\t",
@@ -38,9 +47,10 @@ func NewFormatter(w io.Writer, options ...FormatterOption) Formatter {
3847
type formatter struct {
3948
writer io.Writer
4049

41-
indent string
42-
indentSize int
43-
emitBuiltin bool
50+
indent string
51+
indentSize int
52+
emitBuiltin bool
53+
emitComments bool
4454

4555
padNext bool
4656
lineHead bool
@@ -714,7 +724,7 @@ func (f *formatter) FormatValue(value *ast.Value) {
714724
}
715725

716726
func (f *formatter) FormatCommentGroup(group *ast.CommentGroup) {
717-
if group == nil {
727+
if !f.emitComments || group == nil {
718728
return
719729
}
720730
for _, comment := range group.List {
@@ -723,7 +733,7 @@ func (f *formatter) FormatCommentGroup(group *ast.CommentGroup) {
723733
}
724734

725735
func (f *formatter) FormatComment(comment *ast.Comment) {
726-
if comment == nil {
736+
if !f.emitComments || comment == nil {
727737
return
728738
}
729739
f.WriteString("#").WriteString(comment.Text()).WriteNewline()

‎formatter/formatter_test.go

+123-95
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"flag"
66
"os"
77
"path"
8+
"path/filepath"
89
"testing"
910
"unicode/utf8"
1011

@@ -17,118 +18,145 @@ import (
1718

1819
var update = flag.Bool("u", false, "update golden files")
1920

21+
var optionSets = []struct {
22+
name string
23+
opts []formatter.FormatterOption
24+
}{
25+
{"default", nil},
26+
{"spaceIndent", []formatter.FormatterOption{formatter.WithIndent(" ")}},
27+
{"comments", []formatter.FormatterOption{formatter.WithComments()}},
28+
}
29+
2030
func TestFormatter_FormatSchema(t *testing.T) {
2131
const testSourceDir = "./testdata/source/schema"
2232
const testBaselineDir = "./testdata/baseline/FormatSchema"
2333

24-
executeGoldenTesting(t, &goldenConfig{
25-
SourceDir: testSourceDir,
26-
BaselineFileName: func(cfg *goldenConfig, f os.DirEntry) string {
27-
return path.Join(testBaselineDir, f.Name())
28-
},
29-
Run: func(t *testing.T, cfg *goldenConfig, f os.DirEntry) []byte {
30-
// load stuff
31-
schema, gqlErr := gqlparser.LoadSchema(&ast.Source{
32-
Name: f.Name(),
33-
Input: mustReadFile(path.Join(testSourceDir, f.Name())),
34+
for _, optionSet := range optionSets {
35+
testBaselineDir := filepath.Join(testBaselineDir, optionSet.name)
36+
opts := optionSet.opts
37+
t.Run(optionSet.name, func(t *testing.T) {
38+
executeGoldenTesting(t, &goldenConfig{
39+
SourceDir: testSourceDir,
40+
BaselineFileName: func(cfg *goldenConfig, f os.DirEntry) string {
41+
return path.Join(testBaselineDir, f.Name())
42+
},
43+
Run: func(t *testing.T, cfg *goldenConfig, f os.DirEntry) []byte {
44+
// load stuff
45+
schema, gqlErr := gqlparser.LoadSchema(&ast.Source{
46+
Name: f.Name(),
47+
Input: mustReadFile(path.Join(testSourceDir, f.Name())),
48+
})
49+
if gqlErr != nil {
50+
t.Fatal(gqlErr)
51+
}
52+
53+
// exec format
54+
var buf bytes.Buffer
55+
formatter.NewFormatter(&buf, opts...).FormatSchema(schema)
56+
57+
// validity check
58+
_, gqlErr = gqlparser.LoadSchema(&ast.Source{
59+
Name: f.Name(),
60+
Input: buf.String(),
61+
})
62+
if gqlErr != nil {
63+
t.Log(buf.String())
64+
t.Fatal(gqlErr)
65+
}
66+
67+
return buf.Bytes()
68+
},
3469
})
35-
if gqlErr != nil {
36-
t.Fatal(gqlErr)
37-
}
38-
39-
// exec format
40-
var buf bytes.Buffer
41-
formatter.NewFormatter(&buf).FormatSchema(schema)
42-
43-
// validity check
44-
_, gqlErr = gqlparser.LoadSchema(&ast.Source{
45-
Name: f.Name(),
46-
Input: buf.String(),
47-
})
48-
if gqlErr != nil {
49-
t.Log(buf.String())
50-
t.Fatal(gqlErr)
51-
}
52-
53-
return buf.Bytes()
54-
},
55-
})
70+
})
71+
}
5672
}
5773

5874
func TestFormatter_FormatSchemaDocument(t *testing.T) {
5975
const testSourceDir = "./testdata/source/schema"
6076
const testBaselineDir = "./testdata/baseline/FormatSchemaDocument"
6177

62-
executeGoldenTesting(t, &goldenConfig{
63-
SourceDir: testSourceDir,
64-
BaselineFileName: func(cfg *goldenConfig, f os.DirEntry) string {
65-
return path.Join(testBaselineDir, f.Name())
66-
},
67-
Run: func(t *testing.T, cfg *goldenConfig, f os.DirEntry) []byte {
68-
// load stuff
69-
doc, gqlErr := parser.ParseSchema(&ast.Source{
70-
Name: f.Name(),
71-
Input: mustReadFile(path.Join(testSourceDir, f.Name())),
72-
})
73-
if gqlErr != nil {
74-
t.Fatal(gqlErr)
75-
}
76-
77-
// exec format
78-
var buf bytes.Buffer
79-
formatter.NewFormatter(&buf).FormatSchemaDocument(doc)
80-
81-
// validity check
82-
_, gqlErr = parser.ParseSchema(&ast.Source{
83-
Name: f.Name(),
84-
Input: buf.String(),
78+
for _, optionSet := range optionSets {
79+
testBaselineDir := filepath.Join(testBaselineDir, optionSet.name)
80+
opts := optionSet.opts
81+
t.Run(optionSet.name, func(t *testing.T) {
82+
executeGoldenTesting(t, &goldenConfig{
83+
SourceDir: testSourceDir,
84+
BaselineFileName: func(cfg *goldenConfig, f os.DirEntry) string {
85+
return path.Join(testBaselineDir, f.Name())
86+
},
87+
Run: func(t *testing.T, cfg *goldenConfig, f os.DirEntry) []byte {
88+
// load stuff
89+
doc, gqlErr := parser.ParseSchema(&ast.Source{
90+
Name: f.Name(),
91+
Input: mustReadFile(path.Join(testSourceDir, f.Name())),
92+
})
93+
if gqlErr != nil {
94+
t.Fatal(gqlErr)
95+
}
96+
97+
// exec format
98+
var buf bytes.Buffer
99+
formatter.NewFormatter(&buf, opts...).FormatSchemaDocument(doc)
100+
101+
// validity check
102+
_, gqlErr = parser.ParseSchema(&ast.Source{
103+
Name: f.Name(),
104+
Input: buf.String(),
105+
})
106+
if gqlErr != nil {
107+
t.Log(buf.String())
108+
t.Fatal(gqlErr)
109+
}
110+
111+
return buf.Bytes()
112+
},
85113
})
86-
if gqlErr != nil {
87-
t.Log(buf.String())
88-
t.Fatal(gqlErr)
89-
}
90-
91-
return buf.Bytes()
92-
},
93-
})
114+
})
115+
}
94116
}
95117

96118
func TestFormatter_FormatQueryDocument(t *testing.T) {
97119
const testSourceDir = "./testdata/source/query"
98120
const testBaselineDir = "./testdata/baseline/FormatQueryDocument"
99121

100-
executeGoldenTesting(t, &goldenConfig{
101-
SourceDir: testSourceDir,
102-
BaselineFileName: func(cfg *goldenConfig, f os.DirEntry) string {
103-
return path.Join(testBaselineDir, f.Name())
104-
},
105-
Run: func(t *testing.T, cfg *goldenConfig, f os.DirEntry) []byte {
106-
// load stuff
107-
doc, gqlErr := parser.ParseQuery(&ast.Source{
108-
Name: f.Name(),
109-
Input: mustReadFile(path.Join(testSourceDir, f.Name())),
122+
for _, optionSet := range optionSets {
123+
testBaselineDir := filepath.Join(testBaselineDir, optionSet.name)
124+
opts := optionSet.opts
125+
t.Run(optionSet.name, func(t *testing.T) {
126+
executeGoldenTesting(t, &goldenConfig{
127+
SourceDir: testSourceDir,
128+
BaselineFileName: func(cfg *goldenConfig, f os.DirEntry) string {
129+
return path.Join(testBaselineDir, f.Name())
130+
},
131+
Run: func(t *testing.T, cfg *goldenConfig, f os.DirEntry) []byte {
132+
// load stuff
133+
doc, gqlErr := parser.ParseQuery(&ast.Source{
134+
Name: f.Name(),
135+
Input: mustReadFile(path.Join(testSourceDir, f.Name())),
136+
})
137+
if gqlErr != nil {
138+
t.Fatal(gqlErr)
139+
}
140+
141+
// exec format
142+
var buf bytes.Buffer
143+
formatter.NewFormatter(&buf, opts...).FormatQueryDocument(doc)
144+
145+
// validity check
146+
_, gqlErr = parser.ParseQuery(&ast.Source{
147+
Name: f.Name(),
148+
Input: buf.String(),
149+
})
150+
if gqlErr != nil {
151+
t.Log(buf.String())
152+
t.Fatal(gqlErr)
153+
}
154+
155+
return buf.Bytes()
156+
},
110157
})
111-
if gqlErr != nil {
112-
t.Fatal(gqlErr)
113-
}
114-
115-
// exec format
116-
var buf bytes.Buffer
117-
formatter.NewFormatter(&buf).FormatQueryDocument(doc)
118-
119-
// validity check
120-
_, gqlErr = parser.ParseQuery(&ast.Source{
121-
Name: f.Name(),
122-
Input: buf.String(),
123-
})
124-
if gqlErr != nil {
125-
t.Log(buf.String())
126-
t.Fatal(gqlErr)
127-
}
128-
129-
return buf.Bytes()
130-
},
131-
})
158+
})
159+
}
132160
}
133161

134162
type goldenConfig struct {
@@ -178,11 +206,11 @@ func executeGoldenTesting(t *testing.T, cfg *goldenConfig) {
178206

179207
expected, err := os.ReadFile(expectedFilePath)
180208
if os.IsNotExist(err) {
181-
err = os.MkdirAll(path.Dir(expectedFilePath), 0755)
209+
err = os.MkdirAll(path.Dir(expectedFilePath), 0o755)
182210
if err != nil {
183211
t.Fatal(err)
184212
}
185-
err = os.WriteFile(expectedFilePath, result, 0444)
213+
err = os.WriteFile(expectedFilePath, result, 0o444)
186214
if err != nil {
187215
t.Fatal(err)
188216
}

‎formatter/testdata/baseline/FormatQueryDocument/basic.graphql ‎formatter/testdata/baseline/FormatQueryDocument/comments/basic.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatQueryDocument/field.graphql ‎formatter/testdata/baseline/FormatQueryDocument/comments/field.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatQueryDocument/fragment.graphql ‎formatter/testdata/baseline/FormatQueryDocument/comments/fragment.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatQueryDocument/variable.graphql ‎formatter/testdata/baseline/FormatQueryDocument/comments/variable.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatQueryDocument/default/basic.graphql

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
query FooBarQuery ($after: String!) {
2+
fizzList(first: 100, after: $after) {
3+
nodes {
4+
id
5+
}
6+
}
7+
}

‎formatter/testdata/baseline/FormatQueryDocument/default/field.graphql

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
query {
2+
bar: foo
3+
}

‎formatter/testdata/baseline/FormatQueryDocument/default/fragment.graphql

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
query FooBarQuery ($after: String!) {
2+
fizzList(first: 100, after: $after) {
3+
nodes {
4+
id
5+
... FooFragment
6+
... on Foo {
7+
id
8+
}
9+
... {
10+
id
11+
}
12+
name
13+
}
14+
}
15+
}
16+
fragment FooFragment on Foo {
17+
id
18+
}

‎formatter/testdata/baseline/FormatQueryDocument/default/variable.graphql

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
query ($first: Int = 30, $after: String!) {
2+
searchCats(first: $first, after: $after) {
3+
nodes {
4+
id
5+
name
6+
}
7+
}
8+
}

‎formatter/testdata/baseline/FormatQueryDocument/spaceIndent/basic.graphql

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
query FooBarQuery ($after: String!) {
2+
fizzList(first: 100, after: $after) {
3+
nodes {
4+
id
5+
}
6+
}
7+
}

‎formatter/testdata/baseline/FormatQueryDocument/spaceIndent/field.graphql

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
query {
2+
bar: foo
3+
}

‎formatter/testdata/baseline/FormatQueryDocument/spaceIndent/fragment.graphql

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
query FooBarQuery ($after: String!) {
2+
fizzList(first: 100, after: $after) {
3+
nodes {
4+
id
5+
... FooFragment
6+
... on Foo {
7+
id
8+
}
9+
... {
10+
id
11+
}
12+
name
13+
}
14+
}
15+
}
16+
fragment FooFragment on Foo {
17+
id
18+
}

‎formatter/testdata/baseline/FormatQueryDocument/spaceIndent/variable.graphql

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
query ($first: Int = 30, $after: String!) {
2+
searchCats(first: $first, after: $after) {
3+
nodes {
4+
id
5+
name
6+
}
7+
}
8+
}

‎formatter/testdata/baseline/FormatSchema/comment.graphql ‎formatter/testdata/baseline/FormatSchema/comments/comment.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/definition.graphql ‎formatter/testdata/baseline/FormatSchema/comments/definition.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/description.graphql ‎formatter/testdata/baseline/FormatSchema/comments/description.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/directive.graphql ‎formatter/testdata/baseline/FormatSchema/comments/directive.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/directive_locations.graphql ‎formatter/testdata/baseline/FormatSchema/comments/directive_locations.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/extensions.graphql ‎formatter/testdata/baseline/FormatSchema/comments/extensions.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/field_definition.graphql ‎formatter/testdata/baseline/FormatSchema/comments/field_definition.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/schema.graphql ‎formatter/testdata/baseline/FormatSchema/comments/schema.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/swapi.graphql ‎formatter/testdata/baseline/FormatSchema/comments/swapi.graphql

File renamed without changes.

‎formatter/testdata/baseline/FormatSchema/default/comment.graphql

Whitespace-only changes.

0 commit comments

Comments
 (0)