jmattheis / goverter

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

Unable to convert pointers to direct types like *struct => struct #64

Closed pavelpatrin closed 1 year ago

pavelpatrin commented 1 year ago

Is your feature request related to a problem? Please describe. I'm trying to convert values from *struct to struct automatically, because my models are value based, but gRPC messages are always pointer based. It requires me to write a lot of intermediate converters and next enable them with extend command. Moreover, conversions from struct to *struct also not working as expected, because they don't use field settings for inner method (bug?).

Describe the solution you'd like I want to be able to use the following syntax

type Converter interface {
    FromGrpcMessage(*grpc.Message) domain.Model
}

I already implemented this feature and tried to fix a bug with ignoring original method tags (should be discussed, see PR).

pavelpatrin commented 1 year ago

Hello @jmattheis. I handled all the review notes and hope right now everything in PR got better. Also tested the branch with my own project, it builds well code for conversions between my project value-based models and gRPC message struct pointers.

Also one note that was new for me. Instead of making exported interface methods for internal conversions, it is meaningful to do private methods, like

// goverter:converter
// goverter:ignoreUnexported
// goverter:convertSourcePointers
// goverter:extend (Job|Task).*(From|To)Pb
type Converter interface {
    // JobFromPb converts protobuf message to storage object.
    JobFromPb(value *tmgrpb.Job) (storage.Job, error)

    // JobToPb converts storage object to protobuf message.
    JobToPb(value storage.Job) (*tmgrpb.Job, error)

    // goverter:matchIgnoreCase
    jobFromPb(value tmgrpb.Job) (storage.Job, error)

    // goverter:matchIgnoreCase
    // goverter:map . State | GetStateToPb
    jobToPb(value storage.Job) (tmgrpb.Job, error)
}

I think this way code hints to readers that jobFromPb and jobToPb are utility methods and they can not be deleted only because of missing references from the project codebase.

jmattheis commented 1 year ago

If that works for you, feel free to use it, but it'll produce compile errors when you use certain goverter features, namely https://goverter.jmattheis.de/#/conversion/custom?id=method-with-converter if you've private functions then the generated converter struct won't fit into the Converter interface.

package example

import "strconv"

// goverter:converter
type Converter interface {
    // goverter:map Value | ConvertInt
    Convert(source Input) Output

    convertX(string) string
}

func ConvertInt(c Converter, value int) string {
    return strconv.Itoa(value)
}

type Input struct {
    Value int
}

type Output struct {
    Value string
}
$ goverter ./
$ go build ./generated
# goverter/example/generated
generated/generated.go:11:43: cannot use c (variable of type *ConverterImpl) as example.Converter value in argument to example.ConvertInt: *ConverterImpl does not implement example.Converter (missing method convertX)
pavelpatrin commented 1 year ago

Yes, but it can be bypassed, if the generated file will be stored in the same package as the configuration/extension file.

This can be achieved specifying -output, -packageName and -packagePath together. Also it is required to add //go:generate rm generated.go before. And it looks like a hack.

jmattheis commented 1 year ago

Yeah, you can work around this problem, but it's not officially supported to use unexported functions in converter interfaces.

pavelpatrin commented 1 year ago

Thank you so much!

jmattheis commented 1 year ago

This feature is released with v0.17.0, see https://goverter.jmattheis.de/#/conversion/misc?id=use-zero-value-on-pointer-inconsistency

pavelpatrin commented 1 month ago

@jmattheis we are using this already in several projects and every time I'm thinking about this option name: useZeroValueOnPointerInconsistency.

What do you think, maybe it is reasonable to add alias like useEmptyOnNilPointer for this option? Or even rename it (but remain as a fallback for backward compatibility)?

Maybe after a year you changed your mind?

jmattheis commented 1 month ago

Hmm, I dislike the name useZeroValueOnPointerInconsistency, but useEmptyOnNilPointer could be misleading. If two nilable types are converted e.g. *string to *string then it won't use the zero string value for conversion, even if the source pointer is nil.

pavelpatrin commented 1 month ago

Well, it could be misleading, but for me the useEmptyOnNilPointer says "use empty value for target type, where source type is nil". Target type is reference type - *string and empty (or zero) value for it is nil.

jmattheis commented 2 weeks ago

Sorry for the late-reply.

I'd prefer "ZeroValue" instead of empty because empty has different semantics with slices / maps. E.g. imaging setting useEmptyOnNilPointer with a conversion of []int to []int. I'd expect when the source []int is nil, that we'd use an empty value of the slice []int{} instead.

E.g. from the go docs:

Note that the [zero value](https://go.dev/ref/spec#The_zero_value) for a slice or map type is not the same as an initialized but empty value of the same type.

https://go.dev/ref/spec#Composite_literals

Maybe something like this could be better, but I'm not sure:

goverter:onNilableMismatch useZeroValue

and the default would be

goverter:onNilableMismatch fail
pavelpatrin commented 2 weeks ago

Thank you for the reply. It is wonderful you are thinking about how to improve cosmetics of this.