isAlpha enhancement #1286

mum-never-proud · 2020-04-23T05:29:03Z

resolve #1282

profnandaa

This so far looks good to me, thanks for getting this quick.

Just one thing you forgot, could you update the README too.

mum-never-proud · 2020-04-25T02:10:38Z

@profnandaa thanks for the review, but I see one problem with the above implementation.

since we are allowing the user to pass strings, there are chances that the user might pass some invalid regex characters and the code might break.

we may also experience regex weird behavior, one of the examples is try adding
'h.e.-llo'.replace(/[ -\/]/g, '') paste it in the dev tools you will see that it will also remove dots from the word

so my proposal for strings is to go with the below code

ignore.split('').forEach(ignoredCharacter => str = str.split(ignoredCharacter))

with little tweaks, this code should work but at least it's safe and I hope it gives you the rough idea on what it's doing

validator.js/es/lib/isAlpha.js

Lines 12 to 13 in 5ff4998

    
           } else if (typeof ignore === 'string') { 
        
             str = str.replace(new RegExp("[".concat(ignore, "]"), 'g'), '');

mum-never-proud · 2020-04-27T14:35:35Z

ping @profnandaa

profnandaa · 2020-04-28T08:14:46Z

@mum-never-proud -- I see, nice catch. What if we limited the ignore chars that we support? Pragmatically, I think we should support only the known in-between characters for words, i.e. space, ' and -; Have I left anything?

mum-never-proud · 2020-04-28T14:56:54Z

@profnandaa but again we are narrowing down the use cases, some may raise an issue saying he needs to ignore a back tick or @

it's better to support all the characters in order to avoid reworking for additions

your thoughts?

mum-never-proud · 2020-05-05T03:09:43Z

pong @profnandaa

profnandaa · 2020-05-05T18:30:22Z

@mum-never-proud - makes sense. How about if just support a string with a list of characters to ignore; but not regex for now?

mum-never-proud · 2020-05-06T12:16:36Z

@profnandaa but anyhow we will be using regex to check right?

mum-never-proud · 2020-05-14T11:23:56Z

pong @profnandaa

profnandaa · 2020-05-19T20:41:43Z

@mum-never-proud -- sure, still use regex to check, but don't take regex option from users.

mum-never-proud · 2020-05-29T03:55:41Z

@profnandaa i think we can allow both string and regex, i guess the key solution here will be to escape the regex for string which seemed to work

let me know your thoughts

ps: let me know if you are fin with the approach, so that i can do one final optimization of not modifying the params directly, a bad practice

profnandaa · 2020-05-29T12:17:55Z

@mum-never-proud -- I'm okay with this, thanks.

mum-never-proud · 2020-05-29T13:10:26Z

@profnandaa done,

profnandaa

LGTM You can update the README too?

mum-never-proud · 2020-05-29T15:03:49Z

cool, thanks!

mind providing text?

kinda stuck here

options is optional, valid option is ignore which can either be a String or RegExp to ignore certain characters

anything better than this?

@profnandaa

profnandaa · 2020-05-29T18:11:54Z

I could re-jig it to be similar to the rest, something like:

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.

mum-never-proud · 2020-05-30T01:03:22Z

@profnandaa done!

profnandaa

Thanks! LGTM 🎉

profnandaa · 2020-05-30T07:21:04Z

In case I have missed anything, I will appreciate a second look from @ezkemboi @tux-tn or @rubiin

profnandaa · 2020-06-13T20:21:59Z

ping @tux-tn -- can review please before @mum-never-proud fixes the merge conflicts...

ezkemboi · 2020-07-04T17:02:12Z

src/lib/isAlpha.js

+  assertString(_str);
+
+  let str = _str;
+  const { ignore } = options;


I am with the opinion that ignore characters should be optional.
This actually fails if a user does not pass ignore values (It is not guaranteed that a user needs to have some values ignored).
Also, if I pass options as args: [{ ignore: /[\s/-]/g }], it will fail or I do pass an args: ['en-US', { }] it also fails.
I am open to discussion on way forward.

Cc. @profnandaa

hmm technically i don't see a possibility of failing for any of the above args, may be i can add those

validator.js/src/lib/isAlpha.js

Lines 8 to 10 in a919d99

const { ignore } = options;

if (ignore) {

the above line says that ignore is optional which means, args: ['en-US', { }] should pass

as far as about this [{ ignore: /[\s/-]/g }] we are already asserting first argument to be string

let me know if it makes sense @profnandaa @ezkemboi

ezkemboi

LGTM, just check my comment @mum-never-proud and resolve conflicts.

ezkemboi · 2020-09-28T17:06:14Z

src/lib/isAlpha.js

@@ -1,8 +1,22 @@
 import assertString from './util/assertString';
 import { alpha } from './alpha';

-export default function isAlpha(str, locale = 'en-US') {
-  assertString(str);
+export default function isAlpha(_str, locale = 'en-US', options = {}) {


I am just curious why change str to _str and later do

let str = _str

I thought we could pass str as before and do:

assertString(str);

But, all in all, it looks good to me.

@ezkemboi ah it's not good practice to directly change the arguments, just following best practices, will resolve the conflict in sometime

profnandaa · 2020-10-04T16:35:47Z

@mum-never-proud -- good to see you back, been a minute :) Ping me once you've resolved the conflicts.

mum-never-proud · 2020-10-05T05:13:48Z

@mum-never-proud -- good to see you back, been a minute :) Ping me once you've resolved the conflicts.

heyy, man I totally forgot about this yes I will do today buddy

@profnandaa

ezkemboi · 2020-11-05T02:23:32Z

@mum-never-proud, still waiting for you to resolve conflicts to get this one landing!!!

mum-never-proud · 2020-11-05T02:50:48Z

Hey just a give me a couple of days as my Mac is corrupted I hope to get it resolved by today or worst by tmr and resolve the conflict thanks Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: Ezrqn Kemboi <[email protected]> Sent: Thursday, November 5, 2020 7:53:45 AM To: validatorjs/validator.js <[email protected]> Cc: mum-never-proud <[email protected]>; Mention <[email protected]> Subject: Re: [validatorjs/validator.js] isAlpha enhancement (#1286) @mum-never-proud<https://github.com/mum-never-proud>, still waiting for you to resolve conflicts to get this one landing!!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1286 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AN47NHLW46BTI22575UDP6TSOIEDDANCNFSM4MOX7OSA>.

mum-never-proud · 2020-11-09T03:09:34Z

folks finally laptop is back today i will merge it @profnandaa @ezkemboi

rubiin · 2020-11-09T05:07:41Z

I think we can release a new version after the pr @profnandaa

codecov · 2020-11-09T17:10:04Z

Codecov Report

Merging #1286 (9edb2ca) into master (be8874d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1286   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          96       96           
  Lines        1277     1285    +8     
=======================================
+ Hits         1276     1284    +8     
  Misses          1        1

Impacted Files	Coverage Δ
src/lib/isAlpha.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be8874d...d313ed6. Read the comment docs.

mum-never-proud · 2020-11-10T01:18:18Z

i guess a lot has changed since I created this PR, let me know if there is something I am missing? @profnandaa @ezkemboi

rubiin · 2020-11-10T04:38:27Z

i guess a lot has changed since I created this PR, let me know if there is something I am missing? @profnandaa @ezkemboi

I reviewed and the pr looks okay from my side

ezkemboi · 2020-11-15T10:34:08Z

@profnandaa this is good to land.

patrikgerdin · 2020-12-13T19:58:40Z

Hey, I'm getting an error in typescript (but it is working and validates)

body('field').isAlpha('en-US', {ignore: ' '})

(property) IsAlphaOptions.ignore?: string[] | undefined
Type 'string' is not assignable to type 'string[] | undefined'.ts(2322)

A string[] does not work

body('field').isAlpha('en-US', {ignore: [' ']})

Error: ignore should be instance of a String or RegExp

Am I missing something?

profnandaa · 2020-12-14T10:13:39Z

Yes, ignore is not an [array], should be a "string" or /regex/ Are the docs saying otherwise? ./na

On Sun, Dec 13, 2020 at 10:58 PM Patrik Gerdin ***@***.***> wrote: Hey, I'm getting an error in typescript (but it is working and validates) body('field').isAlpha('en-US', {ignore: ' '}) (property) IsAlphaOptions.ignore?: string[] | undefined Type 'string' is not assignable to type 'string[] | undefined'.ts(2322) A string[] does not work body('field').isAlpha('en-US', {ignore: [' ']}) Error: ignore should be instance of a String or RegExp Am I missing something? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1286 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB7ZELH6AVKVHMH33M56EDSUUMHZANCNFSM4MOX7OSA> .

-- Sent from a tiny device while on the move.

patrikgerdin · 2020-12-14T11:18:45Z

Was looking at the docs here https://github.com/validatorjs/validator.js#validators, hard to see what is the key(s) there.

In options.d.ts

export interface IsAlphaOptions { ignore?: string[]; }

Should be just a string ?

Nice that it works now otherwise.

mum-never-proud added 4 commits April 23, 2020 10:56

isAlpha enhancement

d5291cf

update

7b198ca

testcase

e9bfc4e

revert change

5ff4998

profnandaa reviewed Apr 24, 2020

View reviewed changes

mum-never-proud and others added 3 commits May 29, 2020 08:40

add escape regex

5f385de

Merge branch 'master' into isAlpha-refactor

8d4dff5

revert index

d432f49

enhancements

7629bde

profnandaa reviewed May 29, 2020

View reviewed changes

mum-never-proud and others added 2 commits May 30, 2020 06:28

readme

9cd236a

Merge branch 'master' into isAlpha-refactor

a919d99

profnandaa approved these changes May 30, 2020

View reviewed changes

profnandaa added the needs-more-review label May 30, 2020

ezkemboi requested changes Jul 4, 2020

View reviewed changes

ezkemboi approved these changes Sep 28, 2020

View reviewed changes

Merge branch 'master' into isAlpha-refactor

d313ed6

profnandaa merged commit b569296 into validatorjs:master Nov 15, 2020

profnandaa mentioned this pull request Nov 29, 2020

release staging: Nov 2020 release notes #1535

Merged

This was referenced Mar 12, 2021

chore(deps): bump validator from 9.4.1 to 13.5.2 in /api-server Dman247/freeCodeCamp#185

Open

Bump validator from 13.1.17 to 13.5.2 razee-io/Razeedash-api#825

Closed

TBG-FR mentioned this pull request Jun 22, 2021

question: isAlpha enhancement from validatorjs typestack/class-validator#1144

Closed

isAlpha enhancement #1286

isAlpha enhancement #1286

Conversation

mum-never-proud commented Apr 23, 2020 • edited Loading

profnandaa left a comment

Choose a reason for hiding this comment

mum-never-proud commented Apr 25, 2020

mum-never-proud commented Apr 27, 2020

profnandaa commented Apr 28, 2020

mum-never-proud commented Apr 28, 2020

mum-never-proud commented May 5, 2020

profnandaa commented May 5, 2020

mum-never-proud commented May 6, 2020

mum-never-proud commented May 14, 2020

profnandaa commented May 19, 2020

mum-never-proud commented May 29, 2020 • edited Loading

profnandaa commented May 29, 2020

mum-never-proud commented May 29, 2020

profnandaa left a comment

Choose a reason for hiding this comment

mum-never-proud commented May 29, 2020 • edited Loading

profnandaa commented May 29, 2020

mum-never-proud commented May 30, 2020

profnandaa left a comment

Choose a reason for hiding this comment

profnandaa commented May 30, 2020

profnandaa commented Jun 13, 2020

ezkemboi Jul 4, 2020

Choose a reason for hiding this comment

mum-never-proud Jul 8, 2020

Choose a reason for hiding this comment

ezkemboi left a comment

Choose a reason for hiding this comment

ezkemboi Sep 28, 2020

Choose a reason for hiding this comment

mum-never-proud Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

profnandaa commented Oct 4, 2020

mum-never-proud commented Oct 5, 2020

ezkemboi commented Nov 5, 2020

mum-never-proud commented Nov 5, 2020 via email

mum-never-proud commented Nov 9, 2020

rubiin commented Nov 9, 2020

codecov bot commented Nov 9, 2020 • edited Loading

Codecov Report

mum-never-proud commented Nov 10, 2020

rubiin commented Nov 10, 2020

ezkemboi commented Nov 15, 2020

patrikgerdin commented Dec 13, 2020

profnandaa commented Dec 14, 2020 via email

patrikgerdin commented Dec 14, 2020 • edited Loading

mum-never-proud commented Apr 23, 2020 •

edited

Loading

mum-never-proud commented May 29, 2020 •

edited

Loading

mum-never-proud commented May 29, 2020 •

edited

Loading

mum-never-proud Sep 29, 2020 •

edited

Loading

codecov bot commented Nov 9, 2020 •

edited

Loading

patrikgerdin commented Dec 14, 2020 •

edited

Loading