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

feat(isAlphanumeric): allow usage of options object #2087

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

Conversation

pixelbucket-dev
Copy link

@pixelbucket-dev pixelbucket-dev commented Oct 25, 2022

This PR implements steps 1 and 2 of #1874 for isAlphanumeric and builds upon #2086.

This PR extracts tests for isAlpha into a separate test file ⇾ test/validators/isAlpha.test.js (inspired by #1793).

This PR also renames some constants, so their names follow best-practice naming conventions for constants (UPPER_SNAKE_CASE), but this can be moved to a separate PR or dropped, if wished.

This PR removes unused export statements from src/lib/alpha.js.

This PR moves shared functionality from src/lib/isAlpha.js and src/lib/isAlphanumeric.js to src/lib/alpha.js. If this is not wished, it can be dropped.

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (ed8ce77) compared to base (7b47f53).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2087   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          104       104           
  Lines         2308      2304    -4     
  Branches       578       574    -4     
=========================================
- Hits          2308      2304    -4     
Impacted Files Coverage Δ
src/lib/alpha.js 100.00% <100.00%> (ø)
src/lib/isAfter.js 100.00% <100.00%> (ø)
src/lib/isAlpha.js 100.00% <100.00%> (ø)
src/lib/isAlphanumeric.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

I'm not a fan of the uppercase constants yet, so I'll leave this up to the maintainers. In the end consistency is indeed key so once we do it here we should do it everywhere

**isAlpha(str [, locale, options])** | check if the string contains only letters (a-zA-Z).<br/><br/>Locale is one of `['ar', 'ar-AE', 'ar-BH', 'ar-DZ', 'ar-EG', 'ar-IQ', 'ar-JO', 'ar-KW', 'ar-LB', 'ar-LY', 'ar-MA', 'ar-QA', 'ar-QM', 'ar-SA', 'ar-SD', 'ar-SY', 'ar-TN', 'ar-YE', 'bg-BG', 'bn', 'cs-CZ', 'da-DK', 'de-DE', 'el-GR', 'en-AU', 'en-GB', 'en-HK', 'en-IN', 'en-NZ', 'en-US', 'en-ZA', 'en-ZM', 'es-ES', 'fa-IR', 'fi-FI', 'fr-CA', 'fr-FR', 'he', 'hi-IN', 'hu-HU', 'it-IT', 'ko-KR', 'ja-JP', 'ku-IQ', 'nb-NO', 'nl-NL', 'nn-NO', 'pl-PL', 'pt-BR', 'pt-PT', 'ru-RU', 'si-LK', 'sl-SI', 'sk-SK', 'sr-RS', 'sr-RS@latin', 'sv-SE', 'tr-TR', 'uk-UA']`) and defaults to `en-US`. Locale list is `validator.isAlphaLocales`. options is an optional object that can be supplied with the following key(s): ignore which can either be a String or RegExp of characters to be ignored e.g. " -" will ignore spaces and -'s.
**isAlphanumeric(str [, locale, options])** | check if the string contains only letters and numbers (a-zA-Z0-9).<br/><br/>Locale is one of `['ar', 'ar-AE', 'ar-BH', 'ar-DZ', 'ar-EG', 'ar-IQ', 'ar-JO', 'ar-KW', 'ar-LB', 'ar-LY', 'ar-MA', 'ar-QA', 'ar-QM', 'ar-SA', 'ar-SD', 'ar-SY', 'ar-TN', 'ar-YE', 'bn', 'bg-BG', 'cs-CZ', 'da-DK', 'de-DE', 'el-GR', 'en-AU', 'en-GB', 'en-HK', 'en-IN', 'en-NZ', 'en-US', 'en-ZA', 'en-ZM', 'es-ES', 'fa-IR', 'fi-FI', 'fr-CA', 'fr-FR', 'he', 'hi-IN', 'hu-HU', 'it-IT', 'ko-KR', 'ja-JP','ku-IQ', 'nb-NO', 'nl-NL', 'nn-NO', 'pl-PL', 'pt-BR', 'pt-PT', 'ru-RU', 'si-LK', 'sl-SI', 'sk-SK', 'sr-RS', 'sr-RS@latin', 'sv-SE', 'tr-TR', 'uk-UA']`) and defaults to `en-US`. Locale list is `validator.isAlphanumericLocales`. options is an optional object that can be supplied with the following key(s): ignore which can either be a String or RegExp of characters to be ignored e.g. " -" will ignore spaces and -'s.
**isAfter(str [, options])** | check if the string is a date that's after the specified date.<br/><br/>`options` is an object that defaults to `{ date: new Date() }`.<br/>_Options:_<br/>`date`: Date to compare to. Defaults to `new Date()` (now).
**isAlpha(str [, options])** | check if the string contains only letters (a-zA-Z).<br/><br/>`options` is an object that defaults to `{ locale: 'en-US' }`.<br/><br/>**Options:**<br/>`locale`: The supported locale is one of `['ar', 'ar-AE', 'ar-BH', 'ar-DZ', 'ar-EG', 'ar-IQ', 'ar-JO', 'ar-KW', 'ar-LB', 'ar-LY', 'ar-MA', 'ar-QA', 'ar-QM', 'ar-SA', 'ar-SD', 'ar-SY', 'ar-TN', 'ar-YE', 'bg-BG', 'bn', 'cs-CZ', 'da-DK', 'de-DE', 'el-GR', 'en-AU', 'en-GB', 'en-HK', 'en-IN', 'en-NZ', 'en-US', 'en-ZA', 'en-ZM', 'es-ES', 'fa-IR', 'fi-FI', 'fr-CA', 'fr-FR', 'he', 'hi-IN', 'hu-HU', 'it-IT', 'ko-KR', 'ja-JP', 'ku-IQ', 'nb-NO', 'nl-NL', 'nn-NO', 'pl-PL', 'pt-BR', 'pt-PT', 'ru-RU', 'si-LK', 'sl-SI', 'sk-SK', 'sr-RS', 'sr-RS@latin', 'sv-SE', 'tr-TR', 'uk-UA']`) and defaults to `en-US`. The locale list is `validator.isAlphaLocales`.<br/><br/>`ignore`: (optional) A string or RegExp containing characters to ignore, e.g. `'- /' ` or `/[\s/-]/g` to ignore spaces and "-".
Copy link
Member

Choose a reason for hiding this comment

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

Should we make a separate table for later in this page where we show the different locales? And just refer to that here. Same holds for isAlphanumeric ofc

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, good idea. It's pretty hard to read and redundant as well. Should be a single source of truth, so to speak :D. So, on a second thought, we could just remove the listings here and keep only the reference to validator.isAlphaLocales and validator.isAlphanumericLocales. I am not sure, however, if this reduces transparency. One has to navigated manually though the code base to find the original definition.

Copy link
Member

Choose a reason for hiding this comment

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

I would make a new table for isAlphaLocales, isAlphanumericLocales etc in the README and link to there. So in this table you only reference to that list in the README and mention the default.
People don't have to go through the code that way and that makes this easier to read

@rubiin rubiin requested a review from WikiRik January 23, 2023 07:10
@WikiRik WikiRik added the 🧹 needs-update For PRs that need to be updated before landing label Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 needs-update For PRs that need to be updated before landing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants