jmattheis / goverter

Generate type-safe Go converters by simply defining an interface
https://goverter.jmattheis.de/
MIT License
502 stars 47 forks source link

Ignore compile errors when generating into same package #111

Closed jmattheis closed 9 months ago

jmattheis commented 10 months ago

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 10 months ago

@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 10 months ago

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 10 months ago

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 {
}
phm07 commented 10 months ago

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 10 months ago

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 (https://github.com/jmattheis/goverter/issues/111#issuecomment-1835831040) should be good enough for now.

jmattheis commented 10 months ago

@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.

This branch also contains fixes for #110

phm07 commented 9 months ago

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

jmattheis commented 9 months ago

@pauhull Awesome, thanks for trying it out!

jmattheis commented 9 months ago

Fixed in v1.3.0. See https://goverter.jmattheis.de/reference/build-constraint