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

Added support for arrays of validators and arrays of returned errors #15

Merged
merged 3 commits into from Jan 21, 2012

Conversation

GarethElms
Copy link
Contributor

Hi I've added support for arrays of validators so that I can have different error messages for different types of error. For example if an email address in missing I see "Please enter an email address". If the email address is present but invalid I see "Please enter a valid email address".

If there is a single error then only that string is returned. If there are multiple errors then the array of errors is returned. I haven't thoroughly tested this I have to admit. Is it reasonably easy for you to push through your test suite? I'll do whatever you require of me I don't want to burden you with untested code, but it works for me

MyModel = Backbone.Model.extend(
{

        validation:
        [{
            name:
            {
                required: true,
                msg: "Please enter a player name"
            },
            email:
            {
                required: true,
                msg: "Please enter an email address"
            }
        },
        {
            email:
            {
                pattern: "email",
                msg: "Please enter a valid email"
            }
        }]
    });

It is still possible to pass in a single object for model.validation so it isn't a breaking change

@thedersen
Copy link
Owner

Cool! I do need to put some tests around this before merging it, but it shouldn't be too much work.

However, I would like to suggest a slightly different syntax. Instead of making the validation attribute an array, how about allowing an array of validators on each attribute like this:

MyModel = Backbone.Model.extend({
    validation: {
        name: {
            required: true,
            msg: "Please enter a player name"
        },
        email: [{
            required: true,
            msg: "Please enter an email address"
        },{
            pattern: "email",
            msg: "Please enter a valid email"
        }]
});

Would like your comment on this, as you probably are more familiar with the use case than I am.

@GarethElms
Copy link
Contributor Author

Ah yes that's a much more sensible approach of course. I'll make the change now...

@thedersen
Copy link
Owner

Looking at pulling this in now, and I'm not sure that passing an array of errors to the invalid method makes any sense. I my head it would be better to just return the message for the first validator that is violated.

This is what's requested in issue #16

Any thoughts?

@GarethElms
Copy link
Contributor Author

Hi,

Some forms will have a validation summary at the top or bottom where they'll want to put multiple errors. But this is not the most common usage I don't think. In my current code I'm taking errors[0] anyway because I'm using inline errors where there's only room for a single error per control. A single error being returned would remove the need for this rubbish:

invalid: function( view, attr, errors, selector)
{
    var errorMessage = "";

    if( _.isArray( errors))
    {
        errorMessage =errors[0];
    }
    else
    {
        errorMessage = errors;
    }

You could possibly make this behaviour configurable, so either an array is always returned or not, at least then it would always be consistent (ie; always returns the array even when there is a single error) and if someone has a need for all validation errors then he can do it out of the box. This feels a bit like the dreaded premature optimization though

Perhaps the guy on Issue 16 was thinking that the error 'We need a heading' didn't suit the maxLength validation, but that's solved by allowing multiple validations per field regardless of how many error messages are returned.

Anyway, always returning a single error is absolutely fine by me and, after a little thought, for the best

@thedersen
Copy link
Owner

Cool, I'll go for that then, and add multiple errors later later if the need arises.

@GarethElms
Copy link
Contributor Author

Hi me again sorry to keep bothering you. I've been testing the dev branch and my validation object doesn't work, but it does on my branch :

validation:
{
    whenDate:
    [
        {
            required: true,
            msg: "Please select a date when you are playing your next match"
        },
        function( value, attr, view)
        {
            // code to validate the date
        }
    ]

The function validator isn't called. Are you going to allow this kind of configuration or am I using it incorrectly?

Thanks

@thedersen
Copy link
Owner

No bother:)

I do think it's a bit funky to mix functions and objects like that, so I added a fn validator that allows you to use a function validator in combination with one or more of the buit-in validators.

validation: {
    whenDate: [{
        required: true,
        msg: "Please select a date when you are playing your next match"
    },{
        fn: function( value, attr, view) {
            // code to validate the date
        }
    }]
}

This will also work if you are not using an array of validators

validation: {
    whenDate: {
        required: true,
        fn: function( value, attr, view) {
            // code to validate the date
        }
    }
}

@GarethElms
Copy link
Contributor Author

Ah yes it works cheers :) I can easily have multiple custom functions too which is great :

validation: {
    whenDate: [{
        required: true,
        msg: "Please select a date when you are playing your next match"
    },{
        fn: function( value, attr, view) {
            // code to validate the date
        }
    },{
        fn: function( value, attr, view) {
            // code to validate the date for a different reason
        }
    }]
}

@thedersen
Copy link
Owner

Yup, a nice side effect I didn't think of:)

@thedersen thedersen merged commit fa2e481 into thedersen:master Jan 21, 2012
laiso pushed a commit to laiso/backbone.validation that referenced this pull request May 28, 2015
Change filtered item classes for better styling and added tests for filtered item classes
@ajihyf
Copy link

ajihyf commented Jul 29, 2015

@thedersen
Is this feature not supported in current version? It seems that only the first error message is returned.@_@

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants