Closed Vilsol closed 1 year ago
Slight update:
Looks like defining the convertors below seem to fix it, however that adds a lot of bulk if you have deeply nested structs that are used all over the place.
// goverter:converter
type Converter interface {
// goverter:mapIdentity A
ConvertB(source Input) B
// goverter:mapIdentity B
ConvertBase(source Input) Output
}
Going through the codebase, it seems like the easiest solution would be to add this:
if ctx.IdentityMapping != nil {
namedType := target.NamedType
if target.Pointer {
namedType = target.PointerInner.NamedType
}
if namedType != nil {
prefix := namedType.Obj().Name() + "."
method.IdentityMapping = make(map[string]struct{})
for k, v := range ctx.IdentityMapping {
if strings.HasPrefix(k, prefix) {
method.IdentityMapping[k[len(prefix):]] = v
}
}
}
}
to here: https://github.com/jmattheis/goverter/blob/main/generator/generator.go#L290
That would make sure it will always only trim the target prefix from the expected mappings, and then add all trimmed ones to the new identity mapping.
I am willing to make a PR of this with any changes requested.
Hey,
the current optimal solution would be the one you posted in your update:
// goverter:converter
type Converter interface {
// goverter:mapIdentity A
ConvertB(source Input) B
// goverter:mapIdentity B
ConvertBase(source Input) Output
}
The fix you proposed will work for the easier cases, but with a complex converter structure there would be a lot of undefined / buggy behaviour.
F.ex. what would take precendence, the ignore or the mapIdentity.
// goverter:converter
type Converter interface {
// goverter:mapIdentity B B.A
ConvertBase(source Input) Output
// goverter:ignore A
ConvertBase(source Input) Output
}
In short: goverter comments should not interfere with sub methods, as this may create duplicated definitions.
I don't think there is a good fix for this, without rewriting goverter to support multiple implementation with the same method signature and rewriting it just for this feature would be overkill.
In those cases maybe it would be possible to detect if a definition of the type conversion exists, and then merge their contexts?
Sure, but some definitions contradict each other meaning they can't be merged. like the one from above:
// goverter:converter
type Converter interface {
// goverter:mapIdentity B B.A
ConvertBase(source Input) Output
// goverter:ignore A
ConvertBase(source Input) Output
}
I'm not sure I understand why someone would define a double definition for the same conversion?
It doesn't really matter that someone would define it, my point that it is possible. I don't want goverter to be like "if you don't do something stupid it will probably work" I want it to be solid and predictable this means that there shouldn't be the possibility to miss configure it without any clear errors. The example above is a simple case, but it could be much more complex with a lot of nesting, then it wouldn't be this obvious.
I don't think it is worth to support this feature as merging such definitions isn't that obvious and thus, not that predictable.
Indeed this would add a lot of potential magic. However my issue is that if you are converting from a flat struct to deeply N nested anonymous structure that is reused a lot of times means every single input type will need have N handcrafted extra conversions.
Maybe an option would be to check if there is a handcrafted conversion then use that, if not, resolve a nested mapping. If both are defined, error out?
I also don't fully understand your example. Your both method signatures are identical including the name, input and output types. Assuming that the identical names are a mistake, I don't see how that example applies to the issue at hand? Did you mean to set the bottom method signature to be something like the following?
// goverter:converter
type Converter interface {
// goverter:mapIdentity B B.A
ConvertBase(source Input) Output
// goverter:ignore A
ConvertBaseToB(source Input) B
}
In which case it could error out, as you are trying to use mapIdentity
on a hand-defined conversion.
Did you mean to set the bottom method signature to be something like the following?
Yes.
Maybe an option would be to check if there is a handcrafted conversion then use that, if not, resolve a nested mapping. If both are defined, error out?
This would be possible, but I don't like it. I'd be open to consider including this in goverter if more people want this feature but as of now I don't think it's worth the effort to maintain because it is a little more complicated that your example implementation above. Example which doesn't work:
// goverter:converter
type Converter interface {
// goverter:mapIdentity WithB WithB.WithA
ConvertBase(source Input) Output
}
type Input struct {
Hello string
World int
}
type A struct {
Hello string
}
type B struct {
WithA A
World int
}
type Output struct {
WithB B
}
I'll keep this issue open for more user feedback.
Indeed that example does not work, as I was only working with the presumption of anonymous nested structs.
Maybe an option would be to maintain a parent inheritance tree throughout the context (similar to Go's STD Context) which can then be queried for the current path and then match against an identity mapping?
If it would be implemented, it probably should be done similar to how you can add // goverter:map to methods which take a pointer to a struct. See https://github.com/jmattheis/goverter/commit/e921977029a98515cae5188332a3603a339dad06
@Vilsol This is possible to do in copygen, in a number of ways:
map
directive: manual exampleI've thought about it, with a recent change the pointer behavior described in https://github.com/jmattheis/goverter/issues/7#issuecomment-930391960 was fixed in favor of a more predictable approach, goverter:map isn't inherited to sub methods anymore. Because the goverter generation was adjusted.
Adding this would need such behavior, and I don't think it's worth supporting.
Is your feature request related to a problem? Please describe. Currently there is no way to convert the input to output from this example:
Describe the solution you'd like Being able to add a dot
.
separator for anonymous inherited structs viagoverter:mapIdentity
(likegoverter:map
for regular fields) would make this possible:Describe alternatives you've considered You can resolve a single depth nested struct using this method, but it will error that it is unable to map the identity to the
A
struct: