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

fix: use struct field names in generated code #71

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

0xjac
Copy link
Contributor

@0xjac 0xjac commented Aug 17, 2022

Adds the field names when instantiating the compositeField and
textPreferrer struct in the generated code.

This allows to run the fieldalignment linter without errors on the generated code.

Adds the field names when instantiating the `compositeField` and
`textPreferrer` struct in the generated code.

This allows to run the fieldalignment linter without errors on the
generated code.
@0xjac
Copy link
Contributor Author

0xjac commented Aug 17, 2022

@jschaf I have the following linter errors:

make lint
golangci-lint run
internal/pgdocker/pgdocker.go:20:2: SA1019: "io/ioutil" has been deprecated since Go 1.16: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
	"io/ioutil"
	^
internal/gomod/gomod.go:9:2: SA1019: "io/ioutil" has been deprecated since Go 1.16: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
	"io/ioutil"
	^
internal/parser/interface.go:10:2: SA1019: "io/ioutil" has been deprecated since Go 1.16: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
	"io/ioutil"
	^
make: *** [Makefile:44: lint] Error 1

This seems unrelated to the changes I am submitting here. However I'm happy to add a commit to this PR with a fix if you want me to?

Copy link
Owner

@jschaf jschaf left a comment

Choose a reason for hiding this comment

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

No issues and I'm likely to merge. First, I'd like to understand which lint the change fixes. Seems like fieldalignment is unaffected since the ordering when defining textPreferrer is unchanged.

@@ -229,7 +229,7 @@ type textPreferrer struct {
func (t textPreferrer) PreferredParamFormat() int16 { return pgtype.TextFormatCode }

func (t textPreferrer) NewTypeValue() pgtype.Value {
return textPreferrer{pgtype.NewValue(t.ValueTranscoder).(pgtype.ValueTranscoder), t.typeName}
return textPreferrer{ValueTranscoder: pgtype.NewValue(t.ValueTranscoder).(pgtype.ValueTranscoder), typeName: t.typeName}
Copy link
Owner

Choose a reason for hiding this comment

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

I thought fieldalignment checked for the ordering of fields at the struct definition site. This looks like a lint to use named notation for struct fields instead of the positional notation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct, fieldalignment only checks the struct definition site. But with the -fix flag it can also reorder the fields order. This breaks the instantiation of structs using positional notation. This PR adds the named notation to the few places that do not use it to prevent fieldalignment -fix from breaking the generated code.

Note that with its current definition, textPreferrer doesn't change or break anything. But compositeField does.

@0xjac
Copy link
Contributor Author

0xjac commented Aug 18, 2022

No issues and I'm likely to merge. First, I'd like to understand which lint the change fixes. Seems like fieldalignment is unaffected since the ordering when defining textPreferrer is unchanged.

@jschaf thanks for your quick reply an sorry I was not very clear. I run fieldalignment on the code generated by pggen with the -fix flag. This will change the code by reorganizing the order of fields of every struct at the struct declaration site only in the most space-efficient way. The main candidates here are the struct holding the query params and query outputs (rows).

However fieldalignment is a very simple (dumb) tool and will change the definition of all non-optimized structs it finds (in a given file, package, etc.) But it will not change the order of fields in struct instantiations! Hence the need to use named fields in struct instantiations such that the order is irrelevant. (All of the generated code already does that except for the changes in this PR.)

It's true that in the case of textPreferrer, the fields order is already in the best order so it does not change anything. The main issue is for compositeField which is reorganized by fieldalignment as (defaultVal is now first):

type compositeField struct {
	defaultVal pgtype.ValueTranscoder
	name       string
	typeName   string
}

However I did not see a reason not to fix both. And having the names used for textPreferrer ensures any change to the struct (like adding a field) will force the instantiation to use the field name as well and won't break in the future.

I though this also made sense with the rest of the code which uses the field names in struct instantiations. Reading the last point of the design goals of pggen in CONTRIBUTING.md I thought this was aligned (pun intended):

Generated code should look like a human wrote it. The generated code should be near perfect, including formatting. pggen output doesn't depend on gofmt.

@jschaf jschaf merged commit 176eeb1 into jschaf:main Aug 18, 2022
@jschaf
Copy link
Owner

jschaf commented Aug 18, 2022

Awesome, thank you for the explanation and code. Makes sense and definitely fits in with the goals of generating human-like code.

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.

2 participants