-
Notifications
You must be signed in to change notification settings - Fork 6
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
Validate custom time.Duration types #4
Conversation
Thank you @aaomidi for helping me figure this out. |
…r struct, and use validation tags on a per-Validator basis
field := fl.Field() | ||
param := fl.Param() | ||
v := reflect.ValueOf(struct{}{}) | ||
val := reflect.ValueOf(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.
Is this rename necessary?
val := reflect.ValueOf(struct{}{}) | |
v := reflect.ValueOf(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.
Yes because of the new input parameter v *Validate
. That parameter is needed to meet the function signature defined here. Without the rename I get an a "no new variables on left side" error.
@@ -315,56 +315,56 @@ func isUnique(fl FieldLevel) bool { | |||
} | |||
|
|||
// isMAC is the validation function for validating if the field's value is a valid MAC address. | |||
func isMAC(fl FieldLevel) bool { | |||
func isMAC(v *Validate, fl FieldLevel) bool { |
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.
Here and below, v *Validate
param is being added but never used. I'm not understanding why this is the case.
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.
All bakedInValidators have to match the Func
function signature defined here. Should this parameter be func isMAC(_ *Validate, fl FieldLevel) bool
instead since I don't use v
?
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.
Having not taken the time to fully understand the whole validator package myself, it appears that there's something very weird going on where some things happen at the *Validate
instance level, and others happen at the global level. This existing inconsistency is leading this PR to become very large while attempting to accomplish a seemingly-straightforward goal.
I don't think that this PR is the right place to do a massive refactoring of this library to allow everything to happen at the instance level. If we want to do that (and maybe we do!) it should be as a separate PR, where we can consider multiple different ways of making that happen (e.g. making lots of current top-level helper functions instead be methods on that type, which seems cleaner than having those methods take an instance as their first argument).
Closing in favor of letsencrypt/boulder#7564 |
We can now validate custom
time.Duration
types defined astype MyDuration time.Duration
with all of the baked in tags such asgt
,gte
,eq
,lt
,lte,
etc.In an application, the caller will need to define their custom duration, instantiate a new validator, register the custom duration type, then configure the validation tags where applicable. What that looks like in practice is