-
Notifications
You must be signed in to change notification settings - Fork 228
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
make UnmarshalText take precedence as a converter #58
Conversation
@kisielk ? |
This seems reasonable to me. But I think we need tests for the different orders of precedence of unmarshal methods. |
realizing decoding slices is dealt with separately (https://github.com/gorilla/schema/blob/master/decoder.go#L148) and will require additional changes. example run using this branch: package main
import (
"errors"
"fmt"
"github.com/gorilla/schema"
)
type Foo string
func (f *Foo) UnmarshalText(text []byte) error {
if string(text) != "foo" {
return errors.New("not foo")
}
*f = Foo(text)
return nil
}
type Stuff struct {
Value Foo
}
type SliceStuff struct {
Values []Foo
}
func main() {
stuff := new(Stuff)
values := map[string][]string{
"Value": {"abc"},
}
decoder := schema.NewDecoder()
err := decoder.Decode(stuff, values)
fmt.Println(err) // schema: error converting value for "Value". Details: not foo
fmt.Println(stuff) // &{}
sliceStuff := new(SliceStuff)
values = map[string][]string{
"Values": {"abc", "def"},
}
err = decoder.Decode(sliceStuff, values)
fmt.Println(err) // <nil>
fmt.Println(sliceStuff) // &{[abc def]}
} |
Here's another edge case which happens on master and is not fixed by this pr package main
import (
"errors"
"fmt"
"github.com/gorilla/schema"
)
type Foo struct {
Data string
}
func (f *Foo) UnmarshalText(bytes []byte) error {
return errors.New("always return an error")
}
type Stuff struct {
Value Foo
}
func main() {
stuff := new(Stuff)
values := map[string][]string{
"Value": {""},
}
decoder := schema.NewDecoder()
err := decoder.Decode(stuff, values)
if err != nil {
panic(err)
}
fmt.Printf("%#v\n", stuff) // &main.Stuff{Value:main.Foo{Data:""}}
} |
added some wip on using |
Let me know once you are happy for me to review.
|
@elithrar ive worked on this as much as i want/have time to. up to you if you consider it finished or if you want to make any changes to it. i would have liked to have one single method that handles the precedence for converting values. all of the existing tests pass and i added new tests |
This PR fixes my issue. Can we revisit what it takes to get this merged? |
This is related to this issue. Happy to add tests or make any changes if you agree this makes sense