LegacyStringToSliceHookFunc #99
Conversation
| } | ||
| } | ||
|
|
||
| func LegacyStringToSliceHookFunc(sep string) DecodeHookFunc { |
There was a problem hiding this comment.
@KrzysztofKornalewski-Reply Is this sufficient or do we also need to handle lists as well? I think the example in #64 is "" to []int{}.
There was a problem hiding this comment.
Yes this should be sufficient, as you know from this migration PR:
openbao/openbao#290
There are two errors, and based on my tests and debugging, this approach resolves both. When I manually added LegacyStringToSliceHookFunc, both failed test cases were resolved.
The conversion problem for "1,2,3," for example, occurs because the new Hook implementation of StringToSliceHookFunc() only works when the target is a slice of strings. Previously, it worked for any slices.
About "" to []int{} it also occurs because of workflow has changed in StringToSliceHookFunc().
They also store this solution locally in spf13/viper.
https://github.com/spf13/viper/blob/260b8e9a3c75f49c374251f10fa1529d9e704b6a/viper.go#L1000
There was a problem hiding this comment.
@KrzysztofKornalewski-Reply Ahh, I see now. Makes sense, thank you!
There was a problem hiding this comment.
@KrzysztofKornalewski-Reply In the meantime, you could also open this as a PR against OpenBao I think, and see if that solves the issues with PR 290 :-)
|
@sagikazarmark can I get a review? |
sagikazarmark
left a comment
sagikazarmark
left a comment
There was a problem hiding this comment.
Thanks @KrzysztofKornalewski-Reply !
I had one comment
| } | ||
| } | ||
|
|
||
| func LegacyStringToSliceHookFunc(sep string) DecodeHookFunc { |
There was a problem hiding this comment.
Could you please add a comment why this is added?
I have added this to Viper: https://github.com/spf13/viper/blob/master/viper.go#L1000
I also wonder if it would make sense to call it StringToWeakSlice instead (as in Viper).
There was a problem hiding this comment.
I wanted to migrate from the old mapstructure to the go-viper/mapstruct without additional dependencies. I'm wondering if the official approach requires importing the spy13/viper repository to use the handler you mentioned above.
If yes, then okay.
There was a problem hiding this comment.
But spy13/viper require some additional changes, we don't use Viper structure.
func (v *Viper) defaultDecoderConfig
We would like to use it as a normal function.
There was a problem hiding this comment.
@KrzysztofKornalewski-Reply that's just an example. It's basically the same function that you submitted.
I copied it earlier to Viper to keep backward compatibility.
I linked it as an example to demonstrate:
- what kind of comment I asked for
- what I called the function there
Legacy feels outdated, fragile.
There was a problem hiding this comment.
@sagikazarmark Is there any chance to keep this legacy hook in a repository?
3 participants
Adding LegacyStringToSliceHookFunc for dependent repository.
#100