poppinss / indicative

Indicative is a simple yet powerful data validator for Node.js and browsers. It makes it so simple to write async validations on nested set of data.
https://indicative.adonisjs.com/
MIT License
417 stars 52 forks source link

Pass args to addMessage function #163

Closed ignatevdev closed 6 years ago

ignatevdev commented 6 years ago

This PR allows user to access validation arguments in formatter's addError function. This allows a potential user to move the entire error localization logic to the client-size, which at the moment would not be possible without the custom error template for each validation rule.

Also, I have added a new default formatter called Raw formatter, which makes use of a new argument.

I have covered the new formatter with tests and also added args argument into addError calls for all the other formatters, to make sure that the new argument does not break anything.

All tests pass, a build is generated and the docs also contain the information about the new argument and the new formatter.

I would really appreciate if this PR would be merged asap and then indicative's version would be updated in adonis-validation-provider.

thetutlage commented 6 years ago

Okay so too much is happening here. I like to keep PR's small and focused. Also here are some concerns I have.

  1. Why does indicative needs a Raw formatter?
  2. Can u share an example of how exactly you will use the formatter to translate messages?
  3. Formatter Job should be define the error(s) structure and not their language. Ofcourse, your application can do it, but not something the library itself will do it.

What I am expecting

  1. Remove Raw formatter
  2. An example of how you are using formatter to translate messages
ignatevdev commented 6 years ago

Raw formatter is an example of how and why to use the args parameter.

You might have got it wrong, I am not going to be using the formatter itself to translate the messages, rather than using it's result's structure, you can just look at the tests and see what it produces.

Showing a real example of how I translate the validation errors from the server in my react app would be a bit complicated, but the logic is fairly simple.

I take the validation errors from the response, which are an array of errors grouped by field name. Then I take the errors array for each of the field and use the error's name to find the corresponding translation and substitute the translation parameters with the error arguments.

With js-lingui a translation would look like this

"validation.minLength": {
    "translation": "Field should contain at least {0, plural, one {# character} other {# characters}}"
    ]
  }

And the error will be rendered as such:

<Trans
    id={errors[error.name]}
    values={error.params}
/>

Of course, I did not include this formatter to only match my current implementation, it can be used with any localizaion library such as i18n or gettext.

However, I don't mind if you want it removed, I just thought that it might be useful for other people making rest-api with adonis and also looking for a universal validation errors format.

ignatevdev commented 6 years ago

I have made a commit removing the Raw formatter.

By the way, as I've seen the v4 release can not be used in adonis-validation-provider because of some breaking changes, one of which is the removal of the extend functionality. This change is also not documented on 'Upgrading from v3' page.

But, if you need some help with making the v4 release ready for integration into adonis-validation-provider, I can provide some help.

ignatevdev commented 6 years ago

So are there any other problems with this PR? Can it be merged?

thetutlage commented 6 years ago

The test on travis are failing.

Can u make sure all the tests passes by running npm run test

ignatevdev commented 6 years ago

I've fixed the linting error, but the travis job still fails, because tests output this message pass SAUCE_USERNAME & SAUCE_ACCESS_KEY to run tests on saucelabs and don't actually finish. However, all tests pass locally and in the job as well

thetutlage commented 6 years ago

Looks fine to me. Let me test it manually tonight and then merge it

ignatevdev commented 6 years ago

Alright, thanks!

ignatevdev commented 6 years ago

Any news?

thetutlage commented 6 years ago

Sorry my bad, completely forgot about it.

Would you mind rebasing your branch with the current one, and I will merge it right away?

ignatevdev commented 6 years ago

Done

thetutlage commented 6 years ago

Thanks 😄