-
Notifications
You must be signed in to change notification settings - Fork 0
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
Validation v3 #36
Validation v3 #36
Conversation
…n for RegistrationV2; using switch type to determine which registration type is meant to be validated
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.
Great work! This is coming along, I left a few suggestions.
I think we should reconsider reusing the validation code in order to reduce the volume of changes in the pr.
Also, we should consider following the same pattern for the RegistrationV2
's validation code (i.e. moving the V2 validation code into their own funcs)
Great work, keep it up! 🍻
…c validate function to improve testing
} | ||
} | ||
return nil | ||
} |
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 removed this function entirely since the code is the same for both v1 and v2
validationConfig.go
Outdated
@@ -87,7 +78,7 @@ func buildURLChecker(config ValidatorConfig) (*urlegit.Checker, error) { | |||
func BuildValidators(config ValidatorConfig) ([]Option, error) { | |||
var opts []Option | |||
|
|||
checker, err := buildURLChecker(config) | |||
checker, err := BuildURLChecker(config) | |||
if err != nil { | |||
return nil, err | |||
} |
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.
This function will be updated in a future PR
GetId() string | ||
GetUntil() time.Time | ||
} | ||
|
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.
This has been added to ancla
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.
left a few small suggestions, otherwise LGTM! 🍻
is being added back into ancla
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## denopink/feat/webhooks #36 +/- ##
=========================================================
Coverage ? 91.61%
=========================================================
Files ? 3
Lines ? 298
Branches ? 0
=========================================================
Hits ? 273
Misses ? 25
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
What's Included:
**Need to discuss: