-
Notifications
You must be signed in to change notification settings - Fork 3
Add util function to copy fields between structs #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely! 😍
func CopyFields[A any, B any](a *A, b *B) { | ||
val := reflect.Indirect(reflect.ValueOf(a)) | ||
val2 := reflect.Indirect(reflect.ValueOf(b)) | ||
for i := 0; i < val.Type().NumField(); i++ { | ||
name := val.Type().Field(i).Name | ||
field := val2.FieldByName(name) | ||
if !field.IsValid() { | ||
continue | ||
} | ||
if val.Type().Field(i).Type != field.Type() { | ||
continue | ||
} | ||
if !val.Type().Field(i).IsExported() { | ||
continue | ||
} | ||
field.Set(reflect.Indirect(reflect.ValueOf(a)).FieldByName(name)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea to reduce boilerplate! My main concern with reflection here is that it sacrifices compile-time type safety and could lead to subtle runtime bugs, especially if API struct definitions change. Instead, I’d suggest keeping explicit field copies (or even using simple codegen if we end up needing multiple converters). Adding a quick test using reflection to catch drift between struct fields could be great though and would also help us avoid silent breakages. If we think that this is a common issue and want to go with the codegen approach, I just did a quick search and found some libraries that seem to simplify this, and I'd be happy to prototype it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah same Q can this be solved by embedded structs? As someone who's done quite a few similar things (usually compile time via macros, but confined in a specific domain like SQL objs) this feels like a slippery slope to something akin to lombok, i.e. signs that we're either doing it wrong or using the wrong lang when we need to extend it 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW we can get this free-ish if the structs are protobuf and matching fields have the same type+id. The same []byte
can be decoded by any schema that has the same type+id, although some human still need to verify that the field name & semantics match.
Summary
We should have a function that allows us to copy in fields from one struct to another if they have the same name. This seems like a pretty niche thing - and it probably is. However, what inspired this was the Go Redis client, which has this struct that we want. However, that struct is based on this struct, but has a couple more fancy items. The problem with this is that we really want to use this function, because we don't want to do all the networking code by hand, so we will get an
Options
struct back, but we want aUniversalOptions
struct back instead. So we can either create a copy function which manually doesoptions.Field = universalOptions.Field
once for each of the like 40 fields common to both structs, or do some reflect shenanigans like in the PR here.Test Plan
Unit tests added to verify functionality