Skip to content
This repository has been archived by the owner on Feb 10, 2019. It is now read-only.

External Validators #87

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

christianlent
Copy link
Collaborator

I been using your library for a few months now and I've quite liked it. However, I came across a circumstance where I needed external (functional) validation. I couldn't figure out how to do this with the library as it was (if there's a way, I must have missed the documentation).

This is an extremely bare-bones implementation:
A selector that is checked against each validated field to determine whether validation is run
A validation function that takes the value of the field and returns a Boolean result

Christian Lent added 2 commits March 13, 2014 11:37
var boundValidator = validator.bind(this);
if (!boundValidator(value)) {
validity.valid = false;
validity.failedExternalValidation = true;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a way to specify a message for the validation fail reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch - perhaps named external validators is the solution?

@ericelliott
Copy link
Owner

Thank you very much for your contribution! =)

On the whole, this is good work. For a long time I've planned to revisit this project and move all validation types into their own modules using a mechanism like this.

Could you rename this to addValidator() to prep for that refactor? Basically, instead of calling it "external" anything, it becomes the core mechanism for hooking up validation types. Also, if you'd like to extract one of the built-in validators (like radio or checkbox) and wire it up with addValidator instead of validate(), that would go a long way toward making sure that it supports all the features we need.

@christianlent
Copy link
Collaborator Author

Thanks for the feedback! Let me address your core feedback first. I may make an attempt at refactoring some existing functionality, but my time is running low to work on this right now, so no guarantees. If I were going to refactor an existing piece of functionality, my inclination would be to tackle "validValues" (unless you have an objection).

@christianlent
Copy link
Collaborator Author

Nevermind, "validValues" is not a great refactor candidate.

Added an "options" parameter for custom validators
Added an option "name" for custom validators to identify which
validator caused a fail
Adding a non-function as a validator now causes an exception
@ericelliott
Copy link
Owner

Don't feel obligated. I understand being busy. I appreciate the work you've done so far. This is the closest thing anyone has come up with to my plan to modularize validators. I appreciate the time you already took (including tests).

@christianlent
Copy link
Collaborator Author

OK, I addressed at least some of your feedback, but I haven't yet had time to refactor any existing functionality using the custom validators. I did add an "options" parameter however (currently used for naming validators), but I envision that being used for other really cool things like per-validator errorClass or validClass.

@ericelliott
Copy link
Owner

Good call on the options. I'll take a look later. Working now.

@christianlent
Copy link
Collaborator Author

Anything I can do to help this along? I may have some time this weekend if that's what it needs to be completed.

@ericelliott
Copy link
Owner

Have you managed to refactor any of the existing validations using this approach? I'd like to make sure it supports them all, ideally.

@christianlent
Copy link
Collaborator Author

Hmm, good question. Unfortunately, I haven't yet. I'll take a look next time I have a free weekend. It's definitely a worthy goal. Supposing I actually refactored some, and merely determined that the others are capable of being refactored - would that be sufficient? Also, do all the validations have decent unit tests (so I can tell if my refactoring incidentally breaks anything)?

@ericelliott
Copy link
Owner

Yes, and yes. And I appreciate it! =)

@@ -92,6 +92,9 @@
// Callback stubs
invalidCallback: function () {},
validCallback: function () {},

// Array of External Validator Functions. View the comment for addValidator for more information.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change this so it's not called external, since I'd like this to be the core of how validations work from now on. Thanks for this excellent work. =)

@ericelliott
Copy link
Owner

Any progress? I'd love to merge this work in...

@christianlent
Copy link
Collaborator Author

I'm sorry, I got very busy again! I'll try to take a look this weekend. Still loving h5Validate BTW.

@ericelliott
Copy link
Owner

Thank you 1000x for your work on this. I wish I had more time to invest in this project, because it's a critically important aspect of every app.

@@ -17,6 +17,15 @@
$input.val('');
});

test('Missing required flag:', function () {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "validity" object seemed to have flags set ("tooLong", "patternMismatch", and "valueMissing") and previously were untested. I added unit tests for them, and a method for setting them with the custom validators. The validation option to create such a flag is "validityFailureFlag".

@christianlent
Copy link
Collaborator Author

Let me know if there is anything else I can do to move this along. I'm looking forward to your comments on the changes!

@ericelliott
Copy link
Owner

Sorry it's taken me so long to get around to this. I really need somebody else to take over primary responsibility for maintaining h5Validate if we want it to stay relevant.

@christianlent
Copy link
Collaborator Author

I completely understand and there's certainly no need to apologize! I'm grateful for the work you've already put into the library, and I've personally found it indepensible in one of my current projects.

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

Successfully merging this pull request may close these issues.

2 participants