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

deps: [email protected] closes #6242 coerce input using input + '' #6472

Closed
wants to merge 4 commits into from

Conversation

ajnovice
Copy link

@ajnovice ajnovice commented Feb 9, 2016

No description provided.

@@ -17,19 +17,19 @@ var schema = require('../schema').tables,
// Provide a few custom validators
//
validator.extend('empty', function empty(str) {
return _.isEmpty(str);
return _.isEmpty(str+'');

This comment was marked as abuse.

@acburdine
Copy link
Member

Hey @ajays91, thanks for contributing!

First off, can you fix up the commit as per the outline in #6462 (add the line breaks)? Also, it closes 6462 not 6242 😉

Also, the tests are failing because you don't have spaces between the changes you added. See my line comment.

Lastly, is this the best way to fix the deprecation notices? It seems like a bit of a hack. I'm not saying it isn't, but to me it looks odd.

@ajnovice
Copy link
Author

ajnovice commented Feb 9, 2016

Hey @acburdine
I using the https://github.com/chriso/validator.js/ to solve the bug.
In the above github link, its specified that " Passing input that isn't a string will be an error in an upcoming release. If you're not sure if your input is a string, coerce it using input + ' '. "

My bad for 6242 mistake. 😅

@acburdine
Copy link
Member

@ajays91 you have to use git rebase to squash the commits down into one 😄

something like git rebase HEAD~4 then reword the first one and squash the rest. See the git workflow and ask in Slack if you need more help with it.

Also, I missed this earlier, but your commit doesn't actually update validator.js 😄 Update it to version 4.7.1 (4.7.2 doesn't exist 😉).

@ErisDS
Copy link
Member

ErisDS commented Feb 10, 2016

I agree with @acburdine's original assessment, that this is not the best way to fix the deprecation notices.

Coercing the type is something that should be done as a last resort, in a controlled fashion when you know it's the best option. By doing this in all of our custom validators, all underlying issues in the code base are being actively masked, rather than solved.

Further, it only fixes issues where they use our custom validators. Deprecation notices are still being thrown where non-strings are being passed to other validator functions see example.

I would expect the result of this PR to be an investigation into where the deprecations are coming from, with a set of fixes. Those fixes should only be coercions where absolutely necessary.

@ErisDS
Copy link
Member

ErisDS commented Mar 29, 2016

Closing in favour of #6645

@ErisDS ErisDS closed this Mar 29, 2016
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.

3 participants