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

allow custom validation error messages for all types, not just regex fails #3

Closed
maxcan opened this issue Aug 19, 2013 · 6 comments
Closed

Comments

@maxcan
Copy link

maxcan commented Aug 19, 2013

No description provided.

@aldeed
Copy link
Collaborator

aldeed commented Aug 19, 2013

This is another thing I was planning to do once someone asked for it. Do you have any implementation ideas? I was thinking of providing an API function that allows you to specify the message for each error type, with support for a few basic placeholders. Like:

MySimpleSchema.messages({
  "required": "[label] is required",
  "stringMax": "[label] cannot exceed [max] characters"
});

Might be nice to support field-specific overrides, too. Could just support a field name in the key, separated by a space, like:

MySimpleSchema.messages({
  "required": "[label] is required",
  "required firstName": "I know it's a pain, but we need to insist that you enter your first name.",
  "stringMax": "[label] cannot exceed [max] characters"
});

I can probably implement this later today or tomorrow, unless you have a better idea.

@maxcan
Copy link
Author

maxcan commented Aug 19, 2013

Maybe its better to let the requirements themselves contain messages:

MySchema( {
name: {
type: String
min: {val: 5, message: "must be longer than 5"}
max: {val: 100, message: "your name is too long. deal with it."}
}
...

On Mon, Aug 19, 2013 at 7:49 AM, Eric Dobbertin notifications@github.comwrote:

This is another thing I was planning to do once someone asked for it. Do
you have any implementation ideas? I was thinking of providing an API
function that allows you to specify the message for each error type, with
support for a few basic placeholders. Like:

MySimpleSchema.messages({
"required": "[label] is required",
"stringMax": "[label] cannot exceed [max] characters"});

Might be nice to support field-specific overrides, too. Could just support
a field name in the key, separated by a space, like:

MySimpleSchema.messages({
"required": "[label] is required",
"required firstName": "I know it's a pain, but we need to insist that you enter your first name.",
"stringMax": "[label] cannot exceed [max] characters"});

I can probably implement this later today or tomorrow, unless you have a
better idea.


Reply to this email directly or view it on GitHubhttps://github.com//issues/3#issuecomment-22877369
.

@aldeed
Copy link
Collaborator

aldeed commented Aug 19, 2013

I thought of that, too, but there are a number of downsides:

  • Not every possible message corresponds to a schema key (required and array don't)
  • You'd have to repeat the same message for every field, even if they are all essentially the same
  • Having a separate messages() function makes it possible to rerun the function easily to swap out messages, for example if someone wanted to support multiple languages and pull the messages object from language files.

@aldeed aldeed closed this as completed in 607bdcf Aug 19, 2013
@sbking
Copy link
Contributor

sbking commented Aug 19, 2013

A couple of thoughts on this:

I think there should be both a messages function (and/or messages argument) to define/change default messages, as well as the ability to override with custom messages in the individual requirements. This gives the most flexibility while allowing code to be more terse if desired. Plus it makes more sense to me to put custom messages along with the requirements themselves.

i18n should probably be passed onto a separate i18n package, right? Something that can store strings by keys in different languages and then retrieve the correct string in the current locale.

Also, if required isn't associated with a schema key, then shouldn't the optional property be replaced with "required"? So this could work something like (using the simple-i18n package):

//at some point discover and assign the current locale
locale = "us-en";
...
var i18n = Meteor.I18n().collection;

MySchema = new SimpleSchema({
  "name": {
    required: {true, i18n.findOne({lang: locale, base_str: "schemaNameRequired"})},
    min: 3,
    max: {10, i18n.findOne({lang: locale, base_str: "schemaNameMax"})}
  }
}, {
  required: i18n.findOne({lang: locale, base_str: "schemaRequired"}),
  min: i18n.findOne({lang: locale, base_str: "schemaMin"}),
  max: i18n.findOne({lang: locale, base_str: "schemaMax"})
});

So the key "name" would use the i18n messages for the "us-en" locale under the keys "schemaNameRequired", "schemaNameMax", and "schemaMin". Not that anyone would have to use simple-i18n, but I think this would let you delegate i18n out of this package.

@aldeed
Copy link
Collaborator

aldeed commented Aug 20, 2013

I find it kind of confusing to have the custom messages in one place and the generic messages in another place. I also think it makes the schema a little harder to read. If I'm alone in this, I would not be opposed to supporting the custom message definitions in the schema in addition to the message definitions object.

The reason I chose to use the messages() function rather than the second argument of the constructor is because I can envision people wanting to change the messages on the fly. @sbking's example assumes that locale is going to be static, but sites often let you change it, and whenever this happens, you'd have to redefine the messages, which is easy with the messages() function but you wouldn't want to continually re-construct the SimpleSchema object.

I do agree that externalizing I18N is best.

I know a lot of validation plugins and frameworks make you specify required instead of optional, but for some reason I decided optional was better. I think maybe it was because it seemed safer to have required be the default and make the developer opt out. It's not something I care deeply about, but I'd rather not make breaking changes like that without a good reason.

I any case, this is all implemented in the package already now, so maybe folks can play around with it and see what they do and don't like about the current implementation.

@maxcan
Copy link
Author

maxcan commented Aug 20, 2013

@aldeed fair point on the messages function

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

No branches or pull requests

3 participants