Skip to content

Improve validation when dealing with non-strings#5861

Merged
janmeier merged 2 commits intosequelize:masterfrom
seegno-forks:enhancement/validate-string-and-objects
May 13, 2016
Merged

Improve validation when dealing with non-strings#5861
janmeier merged 2 commits intosequelize:masterfrom
seegno-forks:enhancement/validate-string-and-objects

Conversation

@ruimarinho
Copy link
Copy Markdown
Contributor

Pull Request check-list

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does your issue contain a link to existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Have you added an entry under Future in the changelog?

Description of change

validator@5.0.0+ has stopped coercing input values as strings and, since version 4, the library has been printing deprecation notices everytime a non-string is passed to it.

All validate functions now correctly handle theses cases by delegating this work to lodash functions.

@ruimarinho ruimarinho force-pushed the enhancement/validate-string-and-objects branch from cfbbff8 to 749a24c Compare May 10, 2016 00:02
@ruimarinho
Copy link
Copy Markdown
Contributor Author

Rebased.

INTEGER.prototype.key = INTEGER.key = 'INTEGER';
INTEGER.prototype.validate = function(value) {
if (!Validator.isInt(value)) {
if (_.isString(value) && !Validator.isInt(value) || !_.isString(value) && !_.isInteger(value)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Validator.isInt('' + value)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we coerce to string like validator wants, we avoid have so many checks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In that case I think we can do the same regarding other number-like validations and also the boolean one. WDYT?

@janmeier
Copy link
Copy Markdown
Member

janmeier commented May 10, 2016

LGTM, with a small comment - Also, seems there's still some place in tests where we call extend

@ruimarinho ruimarinho force-pushed the enhancement/validate-string-and-objects branch 2 times, most recently from 08a809a to 683a345 Compare May 11, 2016 12:01
@ruimarinho
Copy link
Copy Markdown
Contributor Author

Rebased and with tests passing. Might make sense to squash 683a345 but I wanted to get your acknowledgement first.

@ruimarinho ruimarinho force-pushed the enhancement/validate-string-and-objects branch from 683a345 to cb90582 Compare May 12, 2016 13:58
@ruimarinho
Copy link
Copy Markdown
Contributor Author

Rebased and ready for merge if you agree with the coerce @janmeier.

@ruimarinho
Copy link
Copy Markdown
Contributor Author

The tests failing are not related to this PR.

@janmeier
Copy link
Copy Markdown
Member

Yep, I agree with the coercion - Needs another rebase, then I promise i'll merge it quickly this this ;)

@janmeier janmeier self-assigned this May 13, 2016
@ruimarinho ruimarinho force-pushed the enhancement/validate-string-and-objects branch from cb90582 to 01a2e05 Compare May 13, 2016 11:44
Rui Marinho added 2 commits May 13, 2016 12:46
validator@5.0.0+ has stopped coercing input values as strings and since
version 4, the library has been printing deprecation notices everytime a
non-string is passed to it.

All `validate` functions now correctly handle theses cases by delegating
this work to lodash functions.
@ruimarinho ruimarinho force-pushed the enhancement/validate-string-and-objects branch from 01a2e05 to 30e9b01 Compare May 13, 2016 11:46
@ruimarinho
Copy link
Copy Markdown
Contributor Author

@janmeier no worries - rebased but still are still failing (not due to my PR though).

@janmeier janmeier merged commit 80e1774 into sequelize:master May 13, 2016
@ruimarinho ruimarinho deleted the enhancement/validate-string-and-objects branch May 13, 2016 15:45
@ruimarinho
Copy link
Copy Markdown
Contributor Author

Cheers. Any chance of a patch release @janmeier? :)

@janmeier
Copy link
Copy Markdown
Member

@ruimarinho
Copy link
Copy Markdown
Contributor Author

Thank you! 🙏

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.

2 participants