Refactor: consolidate field logic in struct. #349
Conversation
jmcarp
force-pushed
the
jmcarp/field-cleanup
branch
from
9b465a5 to
4c1b525
Compare
Currently, we define field descriptions and serialization behavior in several places, with repetitive but also sometime different logic. To make the field generation logic easier to understand and modify, this patch consolidates field descriptions and serialization logic into the TypeFields struct, and derives them from the schema dynamically. We also add tests for these new methods.
jmcarp
force-pushed
the
jmcarp/field-cleanup
branch
from
4c1b525 to
1dafa11
Compare
Rather than deciding which generated fields are represented as pointers in different places in the code, this patch adds an IsPointer() method to TypeFields to consolidate that logic.
jmcarp
force-pushed
the
jmcarp/field-cleanup
branch
from
1dafa11 to
96f0b5c
Compare
| assert.Equal(t, tt.want, got) | ||
| }) | ||
|
|
||
| if diff := cmp.Diff(want, got, cmpIgnoreSchema); diff != "" { |
There was a problem hiding this comment.
This is an example of an assertion we could replace with a comparison of want/got generated code, rather than comparing the intermediate representations. In this case, I don't have a strong preference about which style is better. WDYT @lgfa29?
There was a problem hiding this comment.
Nice! Yeah, this looks good to me.
My general thought was more towards end-to-end testing, like, have a set of OpenAPI JSON spec files and a corresponding set of expected Go generated files and just compare them:
//go:embed test-fixtures/*.json
var jsonSpecs embed.FS
//go:embed test-fixtures/*.go
var goFiles embed.FS
func TestCodeGeneration(t *testing.t) {
for range <each file in jsonSpec> {
spec := <parse JSON file into *openapi3.T>
generateTypes(tmpFile, spec)
expect := <read corresponding goFiles>
assert.Equal(t, <tmpFile content>, expect)
}
}But I don't have enough experience with the codebase yet to know how feasible/useful this would be.
There was a problem hiding this comment.
I think something more like an end-to-end test sounds good, but maybe we'd have a smaller number of bigger tests there. Most of the tests here are making assertions about a small surface area, like whether we're generating a description or type correctly for given inputs, and I don't know that we'd want to generate a large number of those tests as openapi specs and desired output files. So maybe I'll leave these tests alone for now and think about adding end-to-end tests later.
There was a problem hiding this comment.
Actually, maybe we already have the beginnings of this, although just for a single spec, which is defined in go. There are a few golden files like https://github.com/oxidecomputer/oxide.go/blob/main/internal/generate/test_utils/types_output_expected that define expectations for different parts of the generated code. So maybe the approach should be to clean up and expand that kind of testing, in addition to the unit tests we're already writing.
There was a problem hiding this comment.
Oh nice! Yeah, that looks like what I was thinking about. And you're right, we probably only need a handful of these E2E tests. Potentially even just one big file with all different scenarios we expect to encounter 😄
internal/generate/types.go
Outdated
| Name: paramName, | ||
| Type: paramType, | ||
| MarshalKey: p.Value.Name, | ||
| Schema: nil, // nil so StructTag always uses omitempty |
There was a problem hiding this comment.
Would setting OmitDirective: "omitempty" work as well here? instead of Schema: nil? It seems a bit more clear what the intention is without relying on the comment.
There was a problem hiding this comment.
Yep, that works. Changing it.
| Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Description: "A foo field"}}, | ||
| } | ||
| assert.Equal(t, "// Foo is a foo field", f.Description()) | ||
| }) |
There was a problem hiding this comment.
Maybe worth testing FallbackDescription, even though there's a TODO to remove it. It can at least help describe the intended behaviour.
| assert.Equal(t, tt.want, got) | ||
| }) | ||
|
|
||
| if diff := cmp.Diff(want, got, cmpIgnoreSchema); diff != "" { |
There was a problem hiding this comment.
Nice! Yeah, this looks good to me.
My general thought was more towards end-to-end testing, like, have a set of OpenAPI JSON spec files and a corresponding set of expected Go generated files and just compare them:
//go:embed test-fixtures/*.json
var jsonSpecs embed.FS
//go:embed test-fixtures/*.go
var goFiles embed.FS
func TestCodeGeneration(t *testing.t) {
for range <each file in jsonSpec> {
spec := <parse JSON file into *openapi3.T>
generateTypes(tmpFile, spec)
expect := <read corresponding goFiles>
assert.Equal(t, <tmpFile content>, expect)
}
}But I don't have enough experience with the codebase yet to know how feasible/useful this would be.
jmcarp
force-pushed
the
jmcarp/field-cleanup
branch
from
d2be59a to
c4a9a4f
Compare
Currently, we define field descriptions and serialization behavior in several places, with repetitive but also sometime different logic. To make the field generation logic easier to understand and modify, this patch consolidates field descriptions and serialization logic into the TypeFields struct, and derives them from the schema dynamically. We also add tests for these new methods.
Part of #350.
Part of SSE-123.