jmattheis/goverter

Ignore compile errors when generating into same package

Closed this issue · 9 comments

jmattheis commented

Goverter requires type information for generating the conversion methods. When output:file is the defined in the same package as the conversion interface it likely that the generated content will produce compile errors if the source or target types are changed.

To properly work around this issue, you currently have to remove the generated file before running goverter. Example: https://github.com/hetznercloud/hcloud-go/blob/701f3810de06eeffbb6d820abfd6d0c7d6ad5ec1/script/generate_schema.sh

With the addition of #77 this problem will be more likely because it more or less requires that the generated output is in the same package.

Goverter should natively support by ignoring files generated by goverter when loading the packages for conversion. I've two proposals for this:

  1. Remove the generated files before loading packages. Either by removing all files with the generated by github.com/jmattheis/goverter ... comment at the top, or by removing all files defined by output:file.
  2. Add a build constraint to all generated files. E.g. 
    //go:build !goverter_scan
// +build !goverter_scan
    and then load the conversion interface with -tags goverter_scan. This should prevent compile errors originating from goverter generated files.

I'm in favor of the second proposal.

jmattheis commented

@pauhull do you have an opinion regarding this issue? I think this also could remove the need of sed inside the script above because you could define it like this.

keep this inside schema.go

var c converter

and then define the assignment in a file with the build constraitns

//go:build !goverter_scan
// +build !goverter_scan
func init() {
    converter = &converterImpl{}
}

Goverter won't fail, if the impl currently doesn't implement the interface because the file will be ignored when loading packages for generation.

phm07 commented

Yes, that sounds like a really good addition to me! I'm also in favor of the solution using go build tags. It would definitely improve the DX, since the proposed behavior is much more intuitive and would also add support for generation on non-Unix based operating systems in our case.

Maybe there could be a goverter flag (something like assignImplInstanceTo <variable name>) to include the init function in the generated file? Although a separate file with the build flag would probably work fine too. The benefit of including the init function in the generated file would be that there are no compiler errors when the file is not yet generated.

Maybe @jooola and @apricote would like to share their opinions too :)

jmattheis commented

Maybe there could be a goverter flag (something like assignImplInstanceTo ) to include the init function in the generated file?

I think this is too specific to your use-case, but I'd be okay with a more generic solution like adding raw code to the output:file:

// goverter:converter
// goverter:output:file ./output.gen.go
// goverter:output:raw func init() {
// goverter:output:raw     converter = &converterImpl{}
// goverter:output:raw }
type converter interface {
}
  • 👍1
phm07 commented

Yes, that makes sense. I don't know if it's possible, but maybe a solution like this would look cleaner:

// goverter:converter
// goverter:output:file ./output.gen.go
/* goverter:output:raw 
func init() {
    converter = &converterImpl{}
}*/
type converter interface {
}
jmattheis commented

That will be difficult because the current setting only uses lines starting with goverter:. I've moved this into a separate ticket #113 as the solution with build constraints (#111 (comment)) should be good enough for now.

jmattheis commented

@pauhull could you try out the ignore-compile-error branch? Changes: https://github.com/jmattheis/goverter/compare/ignore-compile-error?expand=1

With this, generated files will receive go:build !goverter as build directive and when loading packages -tags goverter is satisfied.

  • go install github.com/jmattheis/goverter/cmd/goverter@ignore-compile-error
  • or go run github.com/jmattheis/goverter/cmd/goverter@ignore-compile-error [pattern]
  • or go get github.com/jmattheis/goverter@ignore-compile-error

This branch also contains fixes for #110

phm07 commented

Yes, the build flags and type assignment now work with the branch you provided 🙂

jmattheis commented

@pauhull Awesome, thanks for trying it out!

  • ❤️1