fix(binding): Expose validator engine used by the default Validator #1277

sudo-suhas · 2018-03-09T04:09:14Z

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 · 2018-03-09T04:09:58Z

@joeybloggs Please have a look at this PR.

codecov · 2018-03-09T04:14:11Z

Codecov Report

Merging #1277 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1277   +/-   ##
=======================================
  Coverage   98.41%   98.41%           
=======================================
  Files          34       34           
  Lines        1762     1762           
=======================================
  Hits         1734     1734           
  Misses         23       23           
  Partials        5        5

Impacted Files	Coverage Δ
binding/binding.go	`100% <ø> (ø)`	⬆️
binding/json.go	`100% <ø> (ø)`	⬆️
binding/default_validator.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65a65c2...9de71a5. Read the comment docs.

deankarn · 2018-03-09T14:55:15Z

Will review this morning

deankarn · 2018-03-09T17:11:00Z

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 Base, just a random name I used in example)

sudo-suhas · 2018-03-10T03:06:57Z

@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.

deankarn · 2018-03-10T04:39:35Z

@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 :) 👍

sudo-suhas · 2018-03-13T03:26:03Z

Ping @appleboy @javierprovecho

sudo-suhas · 2018-03-25T14:34:50Z

@appleboy Could you please comment on this PR? Is there any change required from my side?

appleboy

LGTM, need @javierprovecho approval.

- 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 · 2018-03-28T03:03:30Z

@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.

appleboy · 2018-03-28T03:48:07Z

@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.

sudo-suhas · 2018-03-28T05:15:57Z

joeybloggs can help to review this PR would be great.

@appleboy See #1277 (comment)

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.

appleboy · 2018-03-28T06:41:00Z

README.md

@@ -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


remove the command?

I can merge this PR after removing above command.

tgdn · 2018-03-29T06:06:07Z

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

sudo-suhas · 2018-03-29T06:12:51Z

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.

tgdn · 2018-03-29T07:06:30Z

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)

sudo-suhas force-pushed the feat/expose-validator-engine branch from 1507bf2 to ae09192 Compare March 10, 2018 03:08

sudo-suhas changed the title ~~fix(binding): Expose validator engine used by the dafault Validator~~ fix(binding): Expose validator engine used by the default Validator Mar 10, 2018

sudo-suhas force-pushed the feat/expose-validator-engine branch from ae09192 to 5f50465 Compare March 10, 2018 03:09

appleboy previously approved these changes Mar 25, 2018

View reviewed changes

sudo-suhas added 2 commits March 28, 2018 08:32

fix(binding): Move validator engine getter inside interface

b308c8b

sudo-suhas dismissed appleboy’s stale review via b308c8b March 28, 2018 03:03

sudo-suhas force-pushed the feat/expose-validator-engine branch from 5f50465 to b308c8b Compare March 28, 2018 03:03

appleboy previously approved these changes Mar 28, 2018

View reviewed changes

appleboy added the enhancement label Mar 28, 2018

appleboy added this to the 1.3 milestone Mar 28, 2018

appleboy reviewed Mar 28, 2018

View reviewed changes

docs: rm date cmd from custom validation demo

9de71a5

sudo-suhas dismissed appleboy’s stale review via 9de71a5 March 29, 2018 05:34

appleboy approved these changes Mar 29, 2018

View reviewed changes

appleboy merged commit 6d913fc into gin-gonic:master Mar 29, 2018

sudo-suhas deleted the feat/expose-validator-engine branch March 29, 2018 06:33

sudo-suhas mentioned this pull request Mar 31, 2018

Please repeal commit 26c3f42095b8a1378617eb4756fbf8282cb1ae89 #1211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(binding): Expose validator engine used by the default Validator #1277

fix(binding): Expose validator engine used by the default Validator #1277

sudo-suhas commented Mar 9, 2018

sudo-suhas commented Mar 9, 2018

codecov bot commented Mar 9, 2018 •

edited

Loading

deankarn commented Mar 9, 2018

deankarn commented Mar 9, 2018

sudo-suhas commented Mar 10, 2018

deankarn commented Mar 10, 2018

sudo-suhas commented Mar 13, 2018

sudo-suhas commented Mar 25, 2018

appleboy left a comment

sudo-suhas commented Mar 28, 2018

appleboy commented Mar 28, 2018

sudo-suhas commented Mar 28, 2018

appleboy Mar 28, 2018

appleboy Mar 28, 2018

sudo-suhas Mar 29, 2018

tgdn commented Mar 29, 2018 •

edited

Loading

sudo-suhas commented Mar 29, 2018

tgdn commented Mar 29, 2018

fix(binding): Expose validator engine used by the default Validator #1277

fix(binding): Expose validator engine used by the default Validator #1277

Conversation

sudo-suhas commented Mar 9, 2018

sudo-suhas commented Mar 9, 2018

codecov bot commented Mar 9, 2018 • edited Loading

Codecov Report

deankarn commented Mar 9, 2018

deankarn commented Mar 9, 2018

sudo-suhas commented Mar 10, 2018

deankarn commented Mar 10, 2018

sudo-suhas commented Mar 13, 2018

sudo-suhas commented Mar 25, 2018

appleboy left a comment

Choose a reason for hiding this comment

sudo-suhas commented Mar 28, 2018

appleboy commented Mar 28, 2018

sudo-suhas commented Mar 28, 2018

appleboy Mar 28, 2018

Choose a reason for hiding this comment

appleboy Mar 28, 2018

Choose a reason for hiding this comment

sudo-suhas Mar 29, 2018

Choose a reason for hiding this comment

tgdn commented Mar 29, 2018 • edited Loading

sudo-suhas commented Mar 29, 2018

tgdn commented Mar 29, 2018

codecov bot commented Mar 9, 2018 •

edited

Loading

tgdn commented Mar 29, 2018 •

edited

Loading