Closed asido closed 5 years ago
Wow this great @asido! Thanks so such a thorough PR. I've been thinking about making this exact change for some time now, and am delighted you've not only beat me to it, but added benchmarks to help qualify the change.
For validation to work properly, however, we'd have a bunch of work ahead of us. Lots of code in this package looks like this:
Each validator uses a type assertion before performing a validation operation, and silently skips if the wrong type is presented.
We'd need to flip from a concrete type assertion to using reflection in all of these places. This would definitely be a source of slowdowns, but I think it'd be worth it to be able to use this package on arbitrary go structs. Others have asked for this change as well.
Long story short: I'll happily accept this PR as-is, there's more work to be done. We'd happily accept any subsequent PRs that move us in this direction.
On a clerical note, we have a set of commit message guidelines, for this repo that we ask others to follow. We rely on commit messages to generate change logs. I'd be happy to squash-merge this if you'd prefer not to re-write your commit messages, or you can rebase to have the record properly attributed to you 😄.
Sounds good. I'm changing the commit message.
Shell we also interpret all other 10+ numeric types as number
/integer
?
Let me do an additional PR to fix other validators.
Sounds good. I'm changing the commit message.
delightful. Thanks!
Shell we also interpret all other 10+ numeric types as number/integer?
Yes for sure. Apologies for not clarifying earlier
Let me do an additional PR to fix other validators.
Great. If you'd like to pick a single kind of validator as a proof-of-concept it'll be easier to review, and reduce the chance of duplicated work across all validators. Maybe let's start the validators in object.go
& get a PR merged that updates just those before moving on to the rest
I squashed and changed the message. Further code changes will come another day as another PR if you end up accepting this one as is.
Looks great! Thanks! I'll merge this now
Motivation
Limiting type
object
tomap[string]interface{}
and nothing else is restrictive. It would be understandable if the only possible input is JSON encoded[]byte
. But now we can pass our data structures. Therefore I think the change makes sense.I would also consider mapping every number in
reflect.Kind
to a valid type, but first let me know if this is a change you could think of merging.Benchmark
I benchmarked the implementation using
Go 1.12.7
. The efficiency is identical.Code