-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
TextUnmarshaler support for binding #3045
Conversation
binding/form_mapping.go
Outdated
@@ -198,6 +199,14 @@ func setByForm(value reflect.Value, field reflect.StructField, form map[string][ | |||
} | |||
|
|||
func setWithProperType(val string, value reflect.Value, field reflect.StructField) error { | |||
if value.Kind() != reflect.Struct { |
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.
Why would you need this condition? I think it's perfectly fine to have structs that implement TextUnmarshaler
.
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.
Also, I would consider checking both value
and value.Addr
. It's possible that someone is already using a pointer in the struct that you're binding into:
var out struct {
MyParam *MyType `form:"param"`
}
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.
Ok, so TextUnmarshaler is used by some internal go structs, like time.Time. If we catch that here, we screw up the time handling. TextUnmarshaler is here as a convenience but UnmarshalParam is the safer choice
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.
I added a test for the pointer type and it seems to work as is
@kszafran I think I've addressed all your feedback. Mind taking another look? |
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.
LGTM!
I'm not a maintainer, though. You'll need to ping the maintainers if you want to get this merged.
@thinkerou when you have a chance, could you review/merge/tag another maintainer? |
Looks like an essential feature to have. |
580b8b4
to
1dca789
Compare
@amahiwa-jp I've squashed the commits into a single commit |
ETA for this? |
Updates? |
Any update on when it can be merged? |
Is there anything I can contribute to help get this merged? |
To make it work with arrays case reflect.Array:
if a, e := trySetCustom(val, value); a || e != nil {
return e
}
return errUnknownType |
Any update on this? |
Any update? |
What are the perf changes for this update? How much does it slow down binding for other things? |
@appleboy @thinkerou @javierprovecho Any update on this? |
Again, What is the perf change of this? Have you done the work to assure it does not slow other things down? |
I don't see any performance impact to this. I've been using this in production for a year and a half at this point. Maybe someone could pick it up, update it, and merge it? |
@ssoroka I don't know why I can't review the PR? Can you create a new PR? |
@appleboy this is way off my radar (it's been 2 years and I've switched to huma). Feel free to take the change over and apply it in a new PR. |
I'm happy to help out here if anything is needed. Just let me know. |
Any update on this? |
I can't handle the PR now (maybe no permission to review and trigger unit testing). Please move to New PR: #3933 |
As of this PR you can support any kind of custom type without breaking support for time.Time and other structs that might implement TextUnmarshaler
Closes #2673
Closes #2631