Skip to content
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

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

Merged
merged 3 commits into from
Mar 29, 2018

Conversation

sudo-suhas
Copy link
Contributor

  • 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

Closes #1147, #1211.

@sudo-suhas
Copy link
Contributor Author

@joeybloggs Please have a look at this PR.

@codecov
Copy link

codecov bot commented Mar 9, 2018

Codecov Report

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

Impacted file tree graph

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

deankarn commented Mar 9, 2018

Will review this morning

@deankarn
Copy link

deankarn commented Mar 9, 2018

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
Copy link
Contributor Author

@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 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
@deankarn
Copy link

@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
Copy link
Contributor Author

Ping @appleboy @javierprovecho

@sudo-suhas
Copy link
Contributor Author

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

appleboy
appleboy previously approved these changes Mar 25, 2018
Copy link
Member

@appleboy appleboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

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
appleboy previously approved these changes Mar 28, 2018
@appleboy appleboy added this to the 1.3 milestone Mar 28, 2018
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the command?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can merge this PR after removing above command.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@tgdn
Copy link

tgdn commented Mar 29, 2018

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
Copy link
Contributor Author

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.

@appleboy appleboy merged commit 6d913fc into gin-gonic:master Mar 29, 2018
@sudo-suhas sudo-suhas deleted the feat/expose-validator-engine branch March 29, 2018 06:33
@tgdn
Copy link

tgdn commented Mar 29, 2018

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

tonyhhyip pushed a commit to ysitd-cloud/gin that referenced this pull request Apr 28, 2018
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants