jmattheis / goverter

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

Bug: enum conversion fails with other compatible method signatures defined #141

Closed ewhauser closed 3 months ago

ewhauser commented 3 months ago

Describe the bug When goverter tries to do an enum to enum conversion, it shouldn't use an matching underlying type methods in the conversion

To Reproduce Include the input file that goverter has problems with, and describe the steps you took to trigger the bug

Take the following scenario:

input:
    input.go: |
        package example

        import (
            input "github.com/jmattheis/goverter/execution/input"
            output "github.com/jmattheis/goverter/execution/output"
        )

        // goverter:converter
        // goverter:extend ConvertUnderlying
        type Converter interface {
            // goverter:useUnderlyingTypeMethods
            Convert(input.SqlColor) output.Color
        }

        func ConvertUnderlying(s string) string {
            // This should not be called for an enum to enum conversion
            return ""
        }
    input/sql_enum.go: |
        package input

        type SqlColor string
        const (
            Default SqlColor = "default"
            Green SqlColor   = "green"
            Red   SqlColor   = "red"
        )
    output/enum.go: |
        package output

        type Color string
        const (
            Default Color = "default"
            Green Color   = "green"
            Red   Color   = "red"
        )
success:
    - generated/generated.go: |
        // Code generated by github.com/jmattheis/goverter, DO NOT EDIT.

        package generated

        import (
            execution "github.com/jmattheis/goverter/execution"
            input "github.com/jmattheis/goverter/execution/input"
            output "github.com/jmattheis/goverter/execution/output"
        )

        type ConverterImpl struct{}

        func (c *ConverterImpl) Convert(source input.SqlColor) output.Color {
            return output.Color(execution.ConvertUnderlying(string(source)))
        }

Expected behavior A clear and concise description of what you expected to happen.

extend methods in the namespace shouldn't be used when converting enums.

jmattheis commented 3 months ago

Currently useUnderlyingTypeMethods has precedent over enum matching or actually any other type matching in Goverter. I'm not sure it's that clear-cut that enum matching should take precedent over useUnderlyingTypeMethods and vice-versa.

extend methods in the namespace shouldn't be used when converting enums.

I'm not sure about that. If the extend method matches the enums directly, it should always be used. E.g.

func ConvertEnum(input.SqlColor) output.Color {
    // ...
}

Given that, it's consistent that useUnderlyingTypeMethods also taken precedent over the enum matching but I can see that it's not obvious.

useUnderlyingTypeMethods was initially implemented as an alternative for the enum use-case. So I'd say both settings conflict with each other. What do you think about letting goverter error if an extend method was found via useUnderlyingTypeMethods and the conversion also qualifies for enum conversion?

The conflict can then be resolved by disabling enum for the method:

// goverter:converter
// goverter:extend ConvertUnderlying
// goverter:useUnderlyingTypeMethods
type Converter interface {
    // goverter:enum no
    Convert(input.SqlColor) output.Color
}

or disabling useUnderlyingTypeMethods:

// goverter:converter
// goverter:extend ConvertUnderlying
// goverter:useUnderlyingTypeMethods
type Converter interface {
    // goverter:useUnderlyingTypeMethods no
    Convert(input.SqlColor) output.Color
}
ewhauser commented 3 months ago

@jmattheis I think you're right in that the real problem here is that the settings conflict with each other. The new enum functionality is a much better way to handle these conversions. An error seems reasonable but also don't feel like you need to address just for me.

Thank you!

jmattheis commented 3 months ago

Adding the error should be fairly simple, so I think it's worth to implement it.