tmpfs / async-validate

Asynchronous type validation for node and the browser
Other
314 stars 45 forks source link

Remove field name in custom error messages #59

Closed ersefuril closed 8 years ago

ersefuril commented 8 years ago

I've just followed your example to override default error messages :

var Schema = require('async-validate');
var descriptor = {
      type: 'object',
      fields: {
        product:{type: "string", required: true}
      }
};
var schema = new Schema(descriptor, {messages: messages});
var messages = require('async-validate/messages');
messages.required = "%s is a required field";

In my case, I don't wan't to display field name in error message, so this is what I've done :

messages.required = "REQUIRED !";

But when I'm validating a field (let say "product"), I've got this final error message :

"REQUIRED ! product"

Can't we avoid field name to be concatenated into final error message ?

Please see attached code here : https://tonicdev.com/56fb6a6381168d1100ac0b19/56fb6a6381168d1100ac0b1a

tmpfs commented 8 years ago

In this instance you can use a message function on the rule, it is passed the message and the parameters and you can return a string.

See: https://github.com/tmpfs/async-validate/blob/master/test/fixtures/schema/message-function.js

ersefuril commented 8 years ago

I agree with your proposition, but it means that I'll need to define all error messages for all fields, and I really don't what to do that.

What I want is just overriding default error message, in a global way. So for me it's a small issue, because field name shouldn't be always displayed in error message.

tmpfs commented 8 years ago

I agree it should be easier to do, the current design assumes that the more frequent use case is to indicate the field in the error message, I still feel that is the best default behaviour.

I'll have a think about it and see if I can come up with a better solution.

ersefuril commented 8 years ago

Okay, thanks for your answer.

If I remember, the problem was probably somewhere here (you're passing arguments, here the field name) : https://github.com/tmpfs/async-validate/blob/master/lib/rule.js#L73-L76

tmpfs commented 8 years ago

So if I add an option, let's say {replace: false} and ignore argument replacement when set will that suffice for your use case?

I can't think of a situation when people would want to mix styles and if they do they can define message as a function as noted above.

ersefuril commented 8 years ago

Yeah it could be a solution to add an option ! But can't it be easier for you to edit your format function to not add field name automatically ?

tmpfs commented 8 years ago

As I said before I think the most common use case is to include the field name; remember this isn't just for client-side forms where error messages are shown in context it is a generic solution that can be used for all sorts of validation, for example a web service that validates input data that replies with a message of REQUIRED ! is not particularly useful because it does not say what field was required. Does that make sense to you?

ersefuril commented 8 years ago

I see what you meant and in fact, yes, people will generally want to display field name or no field name at all. So your {replace: false} proposal should be fine !

tmpfs commented 8 years ago

So I ended up implementing this as the option {literal: true} to indicate that messages should be passed through literally.

Updated options documentation here:

https://github.com/tmpfs/async-validate#schema

Test spec:

https://github.com/tmpfs/async-validate/blob/master/test/spec/message-literal.js

Available in the registry as async-validate@0.13.1.

I am closing this as implemented, re-open if you find a bug.

Thanks for your input!

ersefuril commented 8 years ago

Very nice, thanks a lot for your fix, I'll try this soon :)

ersefuril commented 8 years ago

I tried your solution, it's fine but here is a "but" (yeah, sorry).

With {literal: true}, the field name is not displayed anymore in error message, but any other variables are not interpreted (for example; the min rule will print cannot be less than %s).

I know that it can be tricky for you to deal with this, since the user doesn't have control on these field's rules, except if he define one, but maybe we can find a solution ? Thanks in advance !

tmpfs commented 8 years ago

In this case I think you would need to supply a function to handle that case (the mixed scenario we discussed) above, I think literal behaviour won't call the function at the moment.

I think modifying the behaviour to be: pass through literally unless overriden (there is a message string or function on the rule) should do the trick.

I'll take another look.

tmpfs commented 8 years ago

Actually no it looks like a custom message function would be called, I'll add a test case for it.

tmpfs commented 8 years ago

So when a custom message is defined on the rule as a function it is invoked and used (I've updated the literal test spec to check).

So you can set the literal option to get the behaviour you initially wanted and then when you have a requirement for parameter replacement define a message on the rule as a function like this:

https://github.com/tmpfs/async-validate/blob/master/test/fixtures/schema/message-literal.js#L6-L8

Clearly that function can be shared when you have lots of messages that you want to perform replacement on.

ersefuril commented 8 years ago

Okay thanks for your answer !

I tried to combine literal + message function and it seems that literaloption is ignored. Maybe I just need to override message function as you said, ignore name parameter and everything should work ?

Here is a working example : https://tonicdev.com/ersefuril/56fcd50f7a27701100df8307

tmpfs commented 8 years ago

It certainly should work, look at the test spec which tests for both cases:

https://github.com/tmpfs/async-validate/blob/master/test/spec/message-literal.js

The first is passed through literally and the second is with parameters replaced. If you find a bug with the behaviour please supply a test case that reproduces the issue, the link above does not contain enough fields to combine behaviours:

var descriptor = {
      type: 'object',
      fields: {
        product:{type: "string", required: true, whitespace: true, message: errorMessage}
      }
};

Only defines a single field. So to be clear the literal option will only pass through the literal message when the rule does not contain a custom message or function, this is what allows you to bypass replacement when you are supplying messages that do not require replacement (the original issue) and allows you to override for situations where you want parameters to be replaced (for example the min, max messages).

ersefuril commented 8 years ago

Okay I was wrong because it seems that tonicdev doesn't take the latest version of your lib, so we cannot consider my previous example.

I just wrote down some tests, how can I easily give them to you ?

tmpfs commented 8 years ago

Just paste the code as markdown code blocks into this issue please.

ersefuril commented 8 years ago

Okay, I took some time on my side to think about all this stuff.

So, for me there are 3 uses cases :

var fn = function(msg, params) {
    // Remove first params (= name of the field)
    var paramsWithoutField = params.splice(1, params.length - 1);
    return util.format.apply(util, [msg].concat(paramsWithoutField));
};
// Declare your field rule like this...
job: {type: 'string', required: true, whitespace: true, min: 10, message: fn}

Do you agree with these propositions ? If yes, all case are effectively covered.

tmpfs commented 8 years ago

Yes that seems correct, I realize that it might not be ideal but I am certain you can achieve what you want with the existing functionality.

At the moment I don't have any strong ideas on how to improve the message logic and maintain the existing default message behaviour.

ersefuril commented 8 years ago

Alright, since all cases can be covered by writing a custom function or playing with the configuration, everything should be fine.

Thanks for your support, again !