validator.js
validator.js copied to clipboard
validatorjs
[Refactor proposal] Move optional parameters to options object
Currently, there are different ways to define optional parameters. Many take one options object for these, but some have special string parameters like isIP, which has version. Other combine the two like isAlpha, which takes both a locale string and an options object. This is confusing and not consistent throughout the library.
I propose to unify this so that all optional parameters are put in an options object.
This will affect the following validators;
| Current syntax | Proposed new syntax | New options |
|---|---|---|
| isAfter(str [, date]) | isAfter(str [, options]) | date |
| isAlpha(str [, locale, options]) | isAlpha(str [, options]) | locale |
| isAlphanumeric(str [, locale, options]) | isAlphanumeric(str [, options]) | locale |
| isBefore(str [, date]) | isBefore(str [, options]) | date |
| isIdentityCard(str [, locale]) | isIdentityCard(str [, options]) | locale |
| isIP(str [, version]) | isIP(str [, options]) | version |
| isIPRange(str [, version]) | isIPRange(str [, options]) | version |
| isISBN(str [, version]) | isISBN(str [, options]) | version |
| isLicensePlate(str [, locale]) | isLicensePlate(str [, options]) | locale |
| isMobilePhone(str [, locale [, options]]) | isMobilePhone(str [, options]) | locale |
| isRgbColor(str [, includePercentValues]) | isRgbColor(str [, options]) | includePercentValues |
| isUUID(str [, version]) | isUUID(str [, options]) | version |
| matches(str, pattern [, modifiers]) | matches(str, pattern [, options]) | modifiers |
And the following sanitizers;
| Current syntax | Proposed new syntax | New options |
|---|---|---|
| ltrim(input [, chars]) | ltrim(input [, options]) | chars |
| rtrim(input [, chars]) | rtrim(input [, options]) | chars |
| stripLow(input [, keep_new_lines]) | stripLow(input [, options]) | keepNewLines |
| toBoolean(input [, strict]) | toBoolean(input [, options]) | strict |
| toInt(input [, radix]) | toInt(input [, options]) | radix |
| trim(input [, chars]) | trim(input [, options]) | chars |
The idea is that the changes are backwards compatible, so after implementing this both isAlpha(str, locale, options) and isAlpha(str, options) are supported. There will be a message for the first one that the usage is deprecated and the new syntax would be shown.
I am willing to make all these changes and include tests for backwards compatibility, but I'm also willing to rewrite the README file and take up one of them as an example so the community can contribute to the others. EDIT: I forgot about the TypeScript types, I'm not very knowledgeable about those so I will need help with those either way.
What do you think about this proposal? Is this a valid change, or are some of these not needed since they only allow one kind of option? Feel free to discuss.
I strongly prefer the options object, personally. It's easier to read and write when there are several options to choose from, and backwards compatibility is built in.
I can help out with the typescript side of things.
Do you think we should plan a major release in the future where we remove backwards compatibility to the old style? The codebase could get a bit hairy if we maintain backwards compatibility forever, no?
I'll tag @rubiin in here as well before we continue with this. If he approves, I can work on a PR for one of these as an example.
I'll leave the removal of backwards compatibility to the maintainers. For what I've seen in isLength it is not much extra work to keep the old style. What might be nice is to have the tests for the backwards compatibility in a different file to the current implementation. But we'll have to wait until #1793 is merged before we can do that.
@rubiin this might be a nice thing to work on for hacktoberfest, do you think the current maintainers have the capacity to discuss this proposal and review PRs that come out of this? Possibly with other active people in the community (like @braaar ) helping out there as well?
Sorry for the late response, I seemed to have missed it.
I would propose something like the following roadmap;
For step 1 either of the two would work for me;
- Split all tests to 1 file per validator/sanitizer (since a PR for that is almost finished)
- Split the tests for 1 validator away from the big file
This first step is not mandatory, but in my view way overdue and will make it a lot easier to see which tests are for the old behaviour and which ones are for the new behaviour.
Step 2 would be to implement the refactor to one of the validators (isAlpha for example), write a new test suite with the new behaviour and document the new way in the README. Preferably also removing the mention of the old way, but I'm open to document both.
Step 2b would be to add the typings to that in the types repo as well
Step 3 would be to write some new guidelines for contributors. In there we can showcase some "golden standard" examples of validator code and test code. This way new contributions are more consistent with our code style
Step 4 would be to deprecate the old way in a future major release.
Step 5 would be to actually remove the old way in a major release after step 4.
In which major release step 4 and 5 will happen I will leave up to the maintainers but I'm in no hurry for that.
@rubiin is this what you meant for a roadmap? Or did you want to discuss other aspects?
EDIT: Added a new step 3 for contributor guidelines
Solid plan! I think this will increase the quality and consistency of the code base considerably, and make it easier for contributors to make good quality contributions that are consistent with our code style and easier to review.
I would add a step where we write some new guidelines for contributors. In there we can showcase some "golden standard" examples of validator code and test code.
Sorry for the late response, I seemed to have missed it.
I would propose something like the following roadmap;
For step 1 either of the two would work for me;
- Split all tests to 1 file per validator/sanitizer (since a PR for that is almost finished)
- Split the tests for 1 validator away from the big file
This first step is not mandatory, but in my view way overdue and will make it a lot easier to see which tests are for the old behaviour and which ones are for the new behaviour.
Step 2 would be to implement the refactor to one of the validators (
isAlphafor example), write a new test suite with the new behaviour and document the new way in the README. Preferably also removing the mention of the old way, but I'm open to document both.Step 2b would be to add the typings to that in the types repo as well
Step 3 would be to deprecate the old way in a future major release.
Step 4 would be to actually remove the old way in a major release after step 3.
In which major release step 3 and 4 will happen I will leave up to the maintainers but I'm in no hurry for that.
@rubiin is this what you meant for a roadmap? Or did you want to discuss other aspects?
Yeah this is exactly what i meant
I would add a step where we write some new guidelines for contributors. In there we can showcase some "golden standard" examples of validator code and test code.
Good one! I would put that in between the current steps 2 and 3. Will update my comment to reflect that later today
Great initiative @WikiRik . We should probably use hacktoberfest as an opportunity to advance on this. I am ready to help on this (Participating in coordinating/reviewing or contributing to this initiative)
I would like to contribute here. Which file(s) would you suggest I work on a part of this issue? :)
I would like to contribute here. Which file(s) would you suggest I work on a part of this issue? :)
I'll add a column noting which ones have been claimed, but apart from isAfter everything is still open so feel free to claim one. Since my PR has not been merged yet, it is best to branch off from my PR
I would like to contribute here. Which file(s) would you suggest I work on a part of this issue? :)
I'll add a column noting which ones have been claimed, but apart from
isAftereverything is still open so feel free to claim one. Since my PR has not been merged yet, it is best to branch off from my PR
Brilliant, will do :).
@rubiin @tux-tn could you look into #2091 soon? After we've merged that, the basic structure is there to merge the other PRs related to this refactor
@WikiRik I would like to contribute on this as well. Can you assign me one?
@WikiRik I would like to contribute on this as well. Can you assign me one?
Thanks for that! I'll assign you to one once #2091 has been merged since we've noticed that it makes contributing more difficult
Hey @WikiRik
Small correction required the proposed new syntax for isTaxID is isTaxId(str [, options]) and it is not
isTaxID(str, locale [, options]) can you confirm on that.
Small correction required the proposed new syntax for isTaxID is
isTaxId(str [, options])and it is notisTaxID(str, locale [, options])can you confirm on that.
locale is mandatory for isTaxID, right? Then it should not be part of options, at least not in this refactor proposal. This refactor is just for the optional parameters
The first refactor PR has been merged!
@pixelbucket-dev can you rebase your PRs?
If I am not mistaken the list above is missing the isHash validator?
https://github.com/validatorjs/validator.js/blob/master/src/lib/isHash.js
If I am not mistaken the list above is missing the
isHashvalidator? https://github.com/validatorjs/validator.js/blob/master/src/lib/isHash.js
You need to provide an algorithm here so it doesn't fall in the scope of this proposal. This proposal only looks at optional parameters
Changes are done for isTaxId waiting for review and merge.
PR: #2153