fix(binding): Expose validator engine used by the default Validator #1277
Conversation
|
@joeybloggs Please have a look at this PR. |
Codecov Report
@@ Coverage Diff @@
## master #1277 +/- ##
=======================================
Coverage 98.41% 98.41%
=======================================
Files 34 34
Lines 1762 1762
=======================================
Hits 1734 1734
Misses 23 23
Partials 5 5
Continue to review full report at Codecov.
|
|
Will review this morning |
|
Hey @sudo-suhas so looked it over and the only thing I'd recommend is removing the ValidatorEngine If someone overrides the default validator, ValidateEngine will become a source of confusion as it will no longer work as expected. I think that perhaps a slightly modified solution may be more future proof eg. type StructValidator interface {
ValidateStruct(interface{}) error
Base() interface{}
}
v:= binding.Validator.Base().(*validator.Validate)
v.RegisterValidation(...
v.Var(...
etc...The above approach allows it to work with any validation library that's been registered and only requires a small snippet of code in the docs for clarity. (don't have to call it |
|
@joeybloggs, that makes sense, did the suggested change. I was trying to avoid the user having to type cast but like you mentioned, it makes no sense if there is a custom validator implementation. |
sudo-suhas
force-pushed
the
feat/expose-validator-engine
branch
from
1507bf2
to
ae09192
Compare
sudo-suhas
changed the title
sudo-suhas
force-pushed
the
feat/expose-validator-engine
branch
from
ae09192
to
5f50465
Compare
|
@sudo-suhas thanks the changes LGTM I would also have liked to avoid the type cast but the way it’s implemented it seems like the best way. Also it’s not a performance concern as the type cast will be done at setup/config time. Lastly thanks you and the rest of the team for taking this so seriously and I think this will help everyone going forward :) 👍 |
|
Ping @appleboy @javierprovecho |
|
@appleboy Could you please comment on this PR? Is there any change required from my side? |
- Add func ValidatorEngine for returning the underlying validator engine used
in the default StructValidator implementation.
- Remove the function RegisterValidation from the StructValidator interface
which made it immpossible to use a StructValidator implementation without the
validator.v8 library.
- Update and rename test for registering validation
Test{RegisterValidation => ValidatorEngine}.
- Update readme and example for registering custom validation.
- Add example for registering struct level validation.
- Add documentation for the following binding funcs/types:
- Binding interface
- StructValidator interface
- Validator instance
- Binding implementations
- Default func
sudo-suhas
force-pushed
the
feat/expose-validator-engine
branch
from
5f50465
to
b308c8b
Compare
|
@appleboy Would you take into account @joeybloggs's review for this PR and merge it in? @javierprovecho I understand that you might be busy with other things and therefore cannot dedicate enough time for maintenance of this package. However, the project is very popular and I think people might respond if you invited interested people to help out for the same. |
|
@joeybloggs can help to review this PR would be great. We need more maintainer but need @javierprovecho response. I don't have permission to create a new tag for release next version of gin. |
The issue this PR tries to solve is only present on the master branch and has not yet been released. Though a release is probably over due, for now, merging this into master should help people who are trying to use the latest version. |
README.md
Outdated
| @@ -580,13 +584,19 @@ func getBookable(c *gin.Context) { | |||
| ``` | |||
|
|
|||
| ```console | |||
| $ curl "localhost:8085/bookable?check_in=2017-08-16&check_out=2017-08-17" | |||
| $ date +'%m/%d/%Y' | |||
| 03/09/2018 | |||
There was a problem hiding this comment.
I can merge this PR after removing above command.
|
Maybe we should fork this project as the owner does not seem to care, or have enough time, and does not seem to want to give permission |
|
I'd very much prefer avoiding drastic measures such as this one. I am very grateful for all the work the authors and maintainers have put into this. I am merely suggesting that there should be better communication and if needed, they could ask for help. Also, I don't think we should take other people's work for granted. |
|
I completely agree with you, this project is awesome, and the authors have worked very very hard. Although it would probably be a good idea to have more than one person responsible for every change and for every version. Thank you a lot to all of you nonetheless |
…in-gonic#1277) * fix(binding): Expose validator engine used by the default Validator - Add func ValidatorEngine for returning the underlying validator engine used in the default StructValidator implementation. - Remove the function RegisterValidation from the StructValidator interface which made it immpossible to use a StructValidator implementation without the validator.v8 library. - Update and rename test for registering validation Test{RegisterValidation => ValidatorEngine}. - Update readme and example for registering custom validation. - Add example for registering struct level validation. - Add documentation for the following binding funcs/types: - Binding interface - StructValidator interface - Validator instance - Binding implementations - Default func * fix(binding): Move validator engine getter inside interface * docs: rm date cmd from custom validation demo (cherry picked from commit 6d913fc)
ValidatorEnginefor returning the underlying validator engine usedin the default
StructValidatorimplementation.RegisterValidationfrom theStructValidatorinterfacewhich made it immpossible to use a
StructValidatorimplementation without thevalidator.v8library.Test{RegisterValidation => ValidatorEngine}.BindinginterfaceStructValidatorinterfaceValidatorinstanceBindingimplementationsDefaultfuncCloses #1147, #1211.