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

Custom validators for methods #87

Merged
merged 3 commits into from
Nov 28, 2014
Merged

Custom validators for methods #87

merged 3 commits into from
Nov 28, 2014

Conversation

tarmolov
Copy link
Member

I want to keep current normalize system, @twoRoger wants a stricter system (#79).
Let's think about validator mechanism in the library.

Default validation mode is normalize (current way to preprocess parameters). We can add an extra one — strict, for instance. Also a developer has a chance to declare just a custom function for this issue.

This option should be declared to each API method.

Opinions? Cons/pros?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.36%) when pulling c87ed8c on hevil.validator into a26d693 on master.

@twoRoger
Copy link
Contributor

Looks good, in general. Just a couple of notes:

  1. I think there should be a way to specify params validator once per Api instance, not per every method separately.
  2. the second parameter in validators should contain the whole declaration, not just type
  3. imho, the name "validator" implies that it returns true/false

@twoRoger twoRoger mentioned this pull request Nov 26, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling b1f75d4 on hevil.validator into d119062 on master.

* @param {String} [declaration.name] Parameter name.
* @param {String} [declaration.description] Parameter description.
* @param {String} [declaration.type] Parameter type.
* @param {Boolean} [declaration.required] Should the parameter be made obligatory.
* @returns {*}
* @throws HTTP error

Choose a reason for hiding this comment

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

Why HTTP error? )

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling cc2fb1c on hevil.validator into 589872a on master.

param1: {type: 'String'}
},
options: {
paramsValidation: function (paramValue, paramType, paramName) {

Choose a reason for hiding this comment

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

The signature has changed

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling c17f797 on hevil.validator into 589872a on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.68%) when pulling d38d14a on hevil.validator into 589872a on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.68%) when pulling 98b93d2 on hevil.validator into 589872a on master.

@tarmolov
Copy link
Member Author

Any suggestions? Merge?

@watch-the-stars
Copy link

Almost LGTM. The pipe is still unescaped :))

@tarmolov
Copy link
Member Author

GH shows a bad diff. It'is escaped.
https://github.com/baby-loris/bla/blame/hevil.validator/REFERENCE.md#L32

@watch-the-stars
Copy link

There are two of them.

@twoRoger
Copy link
Contributor

🤘

@tarmolov
Copy link
Member Author

OMG :)
Fixed it

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.68%) when pulling edd9dfd on hevil.validator into 589872a on master.

tarmolov added a commit that referenced this pull request Nov 28, 2014
@tarmolov tarmolov merged commit 2c66b35 into master Nov 28, 2014
@tarmolov tarmolov deleted the hevil.validator branch November 28, 2014 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants