fix(isAlpha): fix ح character validation in fa-IR language code (#1400) - #1455

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha
Oct 9, 2020
Merged

fix(isAlpha): fix ح character validation in fa-IR language code (#1400)#1455
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha

Conversation

@fakhrip

@fakhripfakhrip commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

I added some Persian alphabet characters retrieved from wikipedia on this line
Also add new validation regarding the language in the validator.js

This PR specifically fix#1400

Checklist

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

@codecov

codecovBot commented Oct 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1455 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1455 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 96 96 Lines 1275 1276 +1 =======================================
+ Hits 1274 1275 +1 
Misses 1 1 
Impacted FilesCoverage Δ
src/lib/alpha.js100.00% <100.00%> (ø)
src/lib/isPostalCode.js100.00% <0.00%> (ø)
src/lib/isMobilePhone.js100.00% <0.00%> (ø)
src/lib/isPassportNumber.js100.00% <0.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 7916dfd...a83805e. Read the comment docs.

@profnandaa

Copy link
Copy Markdown
Member

Thanks for your contribution! Please rebase your branch with master so as to remove the unrelated changes, then we can review.

@profnandaaprofnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 5, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

I have removed all unnecessary changes, please tell me if i do something wrong here.

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib 🎉 // Just update the README and we should be good to go.

@fakhrip

fakhrip commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Im pretty sure that there are nothing to be updated in the readme file regarding these changes as its already well-documented there (in the readme) @profnandaa

Edit: I've checked all the boxes so its all clear

Comment threadsrc/lib/alpha.js Outdated
'uk-UA': /^[А-ЩЬЮЯЄIЇҐі]+$/i,
'vi-VN': /^[A-ZÀÁẠẢÃÂẦẤẬẨẪĂẰẮẶẲẴĐÈÉẸẺẼÊỀẾỆỂỄÌÍỊỈĨÒÓỌỎÕÔỒỐỘỔỖƠỜỚỢỞỠÙÚỤỦŨƯỪỨỰỬỮỲÝỴỶỸ]+$/i,
'ku-IQ': /^[ئابپتجچحخدرڕزژسشعغفڤقکگلڵمنوۆھەیێيطؤثآإأكضصةظذ]+$/i,
'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can move this to L10 so that it's in alphabetic order.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing!,
ill do it tonight (got to pray first)

@profnandaa

Copy link
Copy Markdown
Member

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

Im sorry but i didnt get it, in which file should i see this ?

@profnandaaprofnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 7, 2020
@profnandaa

Copy link
Copy Markdown
Member

That's alpha.js; let me know if there's anything that should be done.

@fakhrip

fakhrip commented Oct 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we just add this to line 121 in alpha.js to kind of re-override it ?
alpha['fa-IR'] = alpha['fa-IR'];

@profnandaa

Copy link
Copy Markdown
Member

Ok, that works, you can drop in a comment before that.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Done, if there are anything i should do, please tell me 🙏

@profnandaaprofnandaa changed the title feat(isAlpha): feat(isAlpha) fix ح character validation in fa-IR language code (#1400)fix(isAlpha): fix ح character validation in fa-IR language code (#1400)Oct 9, 2020

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib once again! 🎉

@profnandaa
profnandaa merged commit 0c55656 into validatorjs:masterOct 9, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Sure, thank you so much for being good as this is my first legit PR 😃

@profnandaa

Copy link
Copy Markdown
Member

Welcome buddy, looking forward to seeing more from you :)

@tux-tn

Copy link
Copy Markdown
Member

@profnandaa@fakhrip i don't understand the purpose of the change in L124 😅

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

It was overriden by L101 @tux-tn ,
So there i have to "reoverride" it to set the fa-ir to fa-ir (it may sound confusing though, haha)

@tux-tn

Copy link
Copy Markdown
Member

I still don't get it, since alpha array has already been mutated alpha['fa-ir'] received alpha['fa'] content, how is re-overriding it is supposed to bring back the old content?
Logging content before and after the line shows the same value

@fakhrip

Copy link
Copy Markdown
ContributorAuthor
exportconstfarsiLocales=['IR','AF',];for(letlocale,i=0;i<farsiLocales.length;i++){locale=`fa-${farsiLocales[i]}`;alpha[locale]=alpha.fa;// This line changes alpha[fa-IR] content to alpha.faalphanumeric[locale]=alphanumeric.fa;decimal[locale]=decimal.fa;}

And because that the content of alpha.fa and alpha[fa-IR] is different (look below)

'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,fa: /^['0-9آاءأؤئبپتثجچحخدذرزژسشصضطظعغفقکگلمنوهةی۱۲۳۴۵۶۷۸۹۰']+$/i,

It will then be needed to re-override the alpha[fa-IR] that was changed to alpha.fa, to then become alpha[fa-IR] itself

@tux-tn

tux-tn commented Nov 11, 2020

Copy link
Copy Markdown
Member

alpha[locale] = alpha.fa;alpha[fa-IR] has been mutated and contains content of alpha.fa here, right?
Since `alpha[fa_IR] has already been mutated where is stored the old value that you need to reassign?

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Oh wait, i just realized that i was wrong XD, i'm very sorry dude, now i feel like really stupid...

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Do you think i need to fix this in another PR as this one already got merged, or what should i do best ?

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

Sure, you're statement is all good, it was my fault, i'm sorry @tux-tn

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip no problem my friend, thank you for taking the time to answer my questions. It's not a big deal

@profnandaa

Copy link
Copy Markdown
Member

thanks for the due diligence on this @tux-tn

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem in isAlpha( str ,['fa-IR']) not verify ح character

3 participants

@fakhrip@profnandaa@tux-tn
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(isAlpha): fix ح character validation in fa-IR language code (#1400) - #1455

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha
Oct 9, 2020
Merged

fix(isAlpha): fix ح character validation in fa-IR language code (#1400)#1455
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha

Conversation

@fakhrip

@fakhripfakhrip commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

I added some Persian alphabet characters retrieved from wikipedia on this line
Also add new validation regarding the language in the validator.js

This PR specifically fix#1400

Checklist

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

@codecov

codecovBot commented Oct 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1455 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1455 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 96 96 Lines 1275 1276 +1 =======================================
+ Hits 1274 1275 +1 
Misses 1 1 
Impacted FilesCoverage Δ
src/lib/alpha.js100.00% <100.00%> (ø)
src/lib/isPostalCode.js100.00% <0.00%> (ø)
src/lib/isMobilePhone.js100.00% <0.00%> (ø)
src/lib/isPassportNumber.js100.00% <0.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 7916dfd...a83805e. Read the comment docs.

@profnandaa

Copy link
Copy Markdown
Member

Thanks for your contribution! Please rebase your branch with master so as to remove the unrelated changes, then we can review.

@profnandaaprofnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 5, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

I have removed all unnecessary changes, please tell me if i do something wrong here.

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib 🎉 // Just update the README and we should be good to go.

@fakhrip

fakhrip commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Im pretty sure that there are nothing to be updated in the readme file regarding these changes as its already well-documented there (in the readme) @profnandaa

Edit: I've checked all the boxes so its all clear

Comment threadsrc/lib/alpha.js Outdated
'uk-UA': /^[А-ЩЬЮЯЄIЇҐі]+$/i,
'vi-VN': /^[A-ZÀÁẠẢÃÂẦẤẬẨẪĂẰẮẶẲẴĐÈÉẸẺẼÊỀẾỆỂỄÌÍỊỈĨÒÓỌỎÕÔỒỐỘỔỖƠỜỚỢỞỠÙÚỤỦŨƯỪỨỰỬỮỲÝỴỶỸ]+$/i,
'ku-IQ': /^[ئابپتجچحخدرڕزژسشعغفڤقکگلڵمنوۆھەیێيطؤثآإأكضصةظذ]+$/i,
'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can move this to L10 so that it's in alphabetic order.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing!,
ill do it tonight (got to pray first)

@profnandaa

Copy link
Copy Markdown
Member

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

Im sorry but i didnt get it, in which file should i see this ?

@profnandaaprofnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 7, 2020
@profnandaa

Copy link
Copy Markdown
Member

That's alpha.js; let me know if there's anything that should be done.

@fakhrip

fakhrip commented Oct 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we just add this to line 121 in alpha.js to kind of re-override it ?
alpha['fa-IR'] = alpha['fa-IR'];

@profnandaa

Copy link
Copy Markdown
Member

Ok, that works, you can drop in a comment before that.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Done, if there are anything i should do, please tell me 🙏

@profnandaaprofnandaa changed the title feat(isAlpha): feat(isAlpha) fix ح character validation in fa-IR language code (#1400)fix(isAlpha): fix ح character validation in fa-IR language code (#1400)Oct 9, 2020

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib once again! 🎉

@profnandaa
profnandaa merged commit 0c55656 into validatorjs:masterOct 9, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Sure, thank you so much for being good as this is my first legit PR 😃

@profnandaa

Copy link
Copy Markdown
Member

Welcome buddy, looking forward to seeing more from you :)

@tux-tn

Copy link
Copy Markdown
Member

@profnandaa@fakhrip i don't understand the purpose of the change in L124 😅

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

It was overriden by L101 @tux-tn ,
So there i have to "reoverride" it to set the fa-ir to fa-ir (it may sound confusing though, haha)

@tux-tn

Copy link
Copy Markdown
Member

I still don't get it, since alpha array has already been mutated alpha['fa-ir'] received alpha['fa'] content, how is re-overriding it is supposed to bring back the old content?
Logging content before and after the line shows the same value

@fakhrip

Copy link
Copy Markdown
ContributorAuthor
exportconstfarsiLocales=['IR','AF',];for(letlocale,i=0;i<farsiLocales.length;i++){locale=`fa-${farsiLocales[i]}`;alpha[locale]=alpha.fa;// This line changes alpha[fa-IR] content to alpha.faalphanumeric[locale]=alphanumeric.fa;decimal[locale]=decimal.fa;}

And because that the content of alpha.fa and alpha[fa-IR] is different (look below)

'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,fa: /^['0-9آاءأؤئبپتثجچحخدذرزژسشصضطظعغفقکگلمنوهةی۱۲۳۴۵۶۷۸۹۰']+$/i,

It will then be needed to re-override the alpha[fa-IR] that was changed to alpha.fa, to then become alpha[fa-IR] itself

@tux-tn

tux-tn commented Nov 11, 2020

Copy link
Copy Markdown
Member

alpha[locale] = alpha.fa;alpha[fa-IR] has been mutated and contains content of alpha.fa here, right?
Since `alpha[fa_IR] has already been mutated where is stored the old value that you need to reassign?

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Oh wait, i just realized that i was wrong XD, i'm very sorry dude, now i feel like really stupid...

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Do you think i need to fix this in another PR as this one already got merged, or what should i do best ?

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

Sure, you're statement is all good, it was my fault, i'm sorry @tux-tn

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip no problem my friend, thank you for taking the time to answer my questions. It's not a big deal

@profnandaa

Copy link
Copy Markdown
Member

thanks for the due diligence on this @tux-tn

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem in isAlpha( str ,['fa-IR']) not verify ح character

3 participants

@fakhrip@profnandaa@tux-tn
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(isAlpha): fix ح character validation in fa-IR language code (#1400) - #1455

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha
Oct 9, 2020
Merged

fix(isAlpha): fix ح character validation in fa-IR language code (#1400)#1455
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha

Conversation

@fakhrip

@fakhripfakhrip commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

I added some Persian alphabet characters retrieved from wikipedia on this line
Also add new validation regarding the language in the validator.js

This PR specifically fix#1400

Checklist

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

@codecov

codecovBot commented Oct 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1455 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1455 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 96 96 Lines 1275 1276 +1 =======================================
+ Hits 1274 1275 +1 
Misses 1 1 
Impacted FilesCoverage Δ
src/lib/alpha.js100.00% <100.00%> (ø)
src/lib/isPostalCode.js100.00% <0.00%> (ø)
src/lib/isMobilePhone.js100.00% <0.00%> (ø)
src/lib/isPassportNumber.js100.00% <0.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 7916dfd...a83805e. Read the comment docs.

@profnandaa

Copy link
Copy Markdown
Member

Thanks for your contribution! Please rebase your branch with master so as to remove the unrelated changes, then we can review.

@profnandaaprofnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 5, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

I have removed all unnecessary changes, please tell me if i do something wrong here.

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib 🎉 // Just update the README and we should be good to go.

@fakhrip

fakhrip commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Im pretty sure that there are nothing to be updated in the readme file regarding these changes as its already well-documented there (in the readme) @profnandaa

Edit: I've checked all the boxes so its all clear

Comment threadsrc/lib/alpha.js Outdated
'uk-UA': /^[А-ЩЬЮЯЄIЇҐі]+$/i,
'vi-VN': /^[A-ZÀÁẠẢÃÂẦẤẬẨẪĂẰẮẶẲẴĐÈÉẸẺẼÊỀẾỆỂỄÌÍỊỈĨÒÓỌỎÕÔỒỐỘỔỖƠỜỚỢỞỠÙÚỤỦŨƯỪỨỰỬỮỲÝỴỶỸ]+$/i,
'ku-IQ': /^[ئابپتجچحخدرڕزژسشعغفڤقکگلڵمنوۆھەیێيطؤثآإأكضصةظذ]+$/i,
'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can move this to L10 so that it's in alphabetic order.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing!,
ill do it tonight (got to pray first)

@profnandaa

Copy link
Copy Markdown
Member

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

Im sorry but i didnt get it, in which file should i see this ?

@profnandaaprofnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 7, 2020
@profnandaa

Copy link
Copy Markdown
Member

That's alpha.js; let me know if there's anything that should be done.

@fakhrip

fakhrip commented Oct 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we just add this to line 121 in alpha.js to kind of re-override it ?
alpha['fa-IR'] = alpha['fa-IR'];

@profnandaa

Copy link
Copy Markdown
Member

Ok, that works, you can drop in a comment before that.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Done, if there are anything i should do, please tell me 🙏

@profnandaaprofnandaa changed the title feat(isAlpha): feat(isAlpha) fix ح character validation in fa-IR language code (#1400)fix(isAlpha): fix ح character validation in fa-IR language code (#1400)Oct 9, 2020

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib once again! 🎉

@profnandaa
profnandaa merged commit 0c55656 into validatorjs:masterOct 9, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Sure, thank you so much for being good as this is my first legit PR 😃

@profnandaa

Copy link
Copy Markdown
Member

Welcome buddy, looking forward to seeing more from you :)

@tux-tn

Copy link
Copy Markdown
Member

@profnandaa@fakhrip i don't understand the purpose of the change in L124 😅

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

It was overriden by L101 @tux-tn ,
So there i have to "reoverride" it to set the fa-ir to fa-ir (it may sound confusing though, haha)

@tux-tn

Copy link
Copy Markdown
Member

I still don't get it, since alpha array has already been mutated alpha['fa-ir'] received alpha['fa'] content, how is re-overriding it is supposed to bring back the old content?
Logging content before and after the line shows the same value

@fakhrip

Copy link
Copy Markdown
ContributorAuthor
exportconstfarsiLocales=['IR','AF',];for(letlocale,i=0;i<farsiLocales.length;i++){locale=`fa-${farsiLocales[i]}`;alpha[locale]=alpha.fa;// This line changes alpha[fa-IR] content to alpha.faalphanumeric[locale]=alphanumeric.fa;decimal[locale]=decimal.fa;}

And because that the content of alpha.fa and alpha[fa-IR] is different (look below)

'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,fa: /^['0-9آاءأؤئبپتثجچحخدذرزژسشصضطظعغفقکگلمنوهةی۱۲۳۴۵۶۷۸۹۰']+$/i,

It will then be needed to re-override the alpha[fa-IR] that was changed to alpha.fa, to then become alpha[fa-IR] itself

@tux-tn

tux-tn commented Nov 11, 2020

Copy link
Copy Markdown
Member

alpha[locale] = alpha.fa;alpha[fa-IR] has been mutated and contains content of alpha.fa here, right?
Since `alpha[fa_IR] has already been mutated where is stored the old value that you need to reassign?

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Oh wait, i just realized that i was wrong XD, i'm very sorry dude, now i feel like really stupid...

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Do you think i need to fix this in another PR as this one already got merged, or what should i do best ?

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

Sure, you're statement is all good, it was my fault, i'm sorry @tux-tn

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip no problem my friend, thank you for taking the time to answer my questions. It's not a big deal

@profnandaa

Copy link
Copy Markdown
Member

thanks for the due diligence on this @tux-tn

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem in isAlpha( str ,['fa-IR']) not verify ح character

3 participants

@fakhrip@profnandaa@tux-tn
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(isAlpha): fix ح character validation in fa-IR language code (#1400) - #1455

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha
Oct 9, 2020
Merged

fix(isAlpha): fix ح character validation in fa-IR language code (#1400)#1455
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha

Conversation

@fakhrip

@fakhripfakhrip commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

I added some Persian alphabet characters retrieved from wikipedia on this line
Also add new validation regarding the language in the validator.js

This PR specifically fix#1400

Checklist

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

@codecov

codecovBot commented Oct 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1455 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1455 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 96 96 Lines 1275 1276 +1 =======================================
+ Hits 1274 1275 +1 
Misses 1 1 
Impacted FilesCoverage Δ
src/lib/alpha.js100.00% <100.00%> (ø)
src/lib/isPostalCode.js100.00% <0.00%> (ø)
src/lib/isMobilePhone.js100.00% <0.00%> (ø)
src/lib/isPassportNumber.js100.00% <0.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 7916dfd...a83805e. Read the comment docs.

@profnandaa

Copy link
Copy Markdown
Member

Thanks for your contribution! Please rebase your branch with master so as to remove the unrelated changes, then we can review.

@profnandaaprofnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 5, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

I have removed all unnecessary changes, please tell me if i do something wrong here.

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib 🎉 // Just update the README and we should be good to go.

@fakhrip

fakhrip commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Im pretty sure that there are nothing to be updated in the readme file regarding these changes as its already well-documented there (in the readme) @profnandaa

Edit: I've checked all the boxes so its all clear

Comment threadsrc/lib/alpha.js Outdated
'uk-UA': /^[А-ЩЬЮЯЄIЇҐі]+$/i,
'vi-VN': /^[A-ZÀÁẠẢÃÂẦẤẬẨẪĂẰẮẶẲẴĐÈÉẸẺẼÊỀẾỆỂỄÌÍỊỈĨÒÓỌỎÕÔỒỐỘỔỖƠỜỚỢỞỠÙÚỤỦŨƯỪỨỰỬỮỲÝỴỶỸ]+$/i,
'ku-IQ': /^[ئابپتجچحخدرڕزژسشعغفڤقکگلڵمنوۆھەیێيطؤثآإأكضصةظذ]+$/i,
'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can move this to L10 so that it's in alphabetic order.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing!,
ill do it tonight (got to pray first)

@profnandaa

Copy link
Copy Markdown
Member

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

Im sorry but i didnt get it, in which file should i see this ?

@profnandaaprofnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 7, 2020
@profnandaa

Copy link
Copy Markdown
Member

That's alpha.js; let me know if there's anything that should be done.

@fakhrip

fakhrip commented Oct 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we just add this to line 121 in alpha.js to kind of re-override it ?
alpha['fa-IR'] = alpha['fa-IR'];

@profnandaa

Copy link
Copy Markdown
Member

Ok, that works, you can drop in a comment before that.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Done, if there are anything i should do, please tell me 🙏

@profnandaaprofnandaa changed the title feat(isAlpha): feat(isAlpha) fix ح character validation in fa-IR language code (#1400)fix(isAlpha): fix ح character validation in fa-IR language code (#1400)Oct 9, 2020

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib once again! 🎉

@profnandaa
profnandaa merged commit 0c55656 into validatorjs:masterOct 9, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Sure, thank you so much for being good as this is my first legit PR 😃

@profnandaa

Copy link
Copy Markdown
Member

Welcome buddy, looking forward to seeing more from you :)

@tux-tn

Copy link
Copy Markdown
Member

@profnandaa@fakhrip i don't understand the purpose of the change in L124 😅

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

It was overriden by L101 @tux-tn ,
So there i have to "reoverride" it to set the fa-ir to fa-ir (it may sound confusing though, haha)

@tux-tn

Copy link
Copy Markdown
Member

I still don't get it, since alpha array has already been mutated alpha['fa-ir'] received alpha['fa'] content, how is re-overriding it is supposed to bring back the old content?
Logging content before and after the line shows the same value

@fakhrip

Copy link
Copy Markdown
ContributorAuthor
exportconstfarsiLocales=['IR','AF',];for(letlocale,i=0;i<farsiLocales.length;i++){locale=`fa-${farsiLocales[i]}`;alpha[locale]=alpha.fa;// This line changes alpha[fa-IR] content to alpha.faalphanumeric[locale]=alphanumeric.fa;decimal[locale]=decimal.fa;}

And because that the content of alpha.fa and alpha[fa-IR] is different (look below)

'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,fa: /^['0-9آاءأؤئبپتثجچحخدذرزژسشصضطظعغفقکگلمنوهةی۱۲۳۴۵۶۷۸۹۰']+$/i,

It will then be needed to re-override the alpha[fa-IR] that was changed to alpha.fa, to then become alpha[fa-IR] itself

@tux-tn

tux-tn commented Nov 11, 2020

Copy link
Copy Markdown
Member

alpha[locale] = alpha.fa;alpha[fa-IR] has been mutated and contains content of alpha.fa here, right?
Since `alpha[fa_IR] has already been mutated where is stored the old value that you need to reassign?

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Oh wait, i just realized that i was wrong XD, i'm very sorry dude, now i feel like really stupid...

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Do you think i need to fix this in another PR as this one already got merged, or what should i do best ?

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

Sure, you're statement is all good, it was my fault, i'm sorry @tux-tn

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip no problem my friend, thank you for taking the time to answer my questions. It's not a big deal

@profnandaa

Copy link
Copy Markdown
Member

thanks for the due diligence on this @tux-tn

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem in isAlpha( str ,['fa-IR']) not verify ح character

3 participants

@fakhrip@profnandaa@tux-tn
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(isAlpha): fix ح character validation in fa-IR language code (#1400) - #1455

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha
Oct 9, 2020
Merged

fix(isAlpha): fix ح character validation in fa-IR language code (#1400)#1455
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha

Conversation

@fakhrip

@fakhripfakhrip commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

I added some Persian alphabet characters retrieved from wikipedia on this line
Also add new validation regarding the language in the validator.js

This PR specifically fix#1400

Checklist

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

@codecov

codecovBot commented Oct 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1455 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1455 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 96 96 Lines 1275 1276 +1 =======================================
+ Hits 1274 1275 +1 
Misses 1 1 
Impacted FilesCoverage Δ
src/lib/alpha.js100.00% <100.00%> (ø)
src/lib/isPostalCode.js100.00% <0.00%> (ø)
src/lib/isMobilePhone.js100.00% <0.00%> (ø)
src/lib/isPassportNumber.js100.00% <0.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 7916dfd...a83805e. Read the comment docs.

@profnandaa

Copy link
Copy Markdown
Member

Thanks for your contribution! Please rebase your branch with master so as to remove the unrelated changes, then we can review.

@profnandaaprofnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 5, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

I have removed all unnecessary changes, please tell me if i do something wrong here.

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib 🎉 // Just update the README and we should be good to go.

@fakhrip

fakhrip commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Im pretty sure that there are nothing to be updated in the readme file regarding these changes as its already well-documented there (in the readme) @profnandaa

Edit: I've checked all the boxes so its all clear

Comment threadsrc/lib/alpha.js Outdated
'uk-UA': /^[А-ЩЬЮЯЄIЇҐі]+$/i,
'vi-VN': /^[A-ZÀÁẠẢÃÂẦẤẬẨẪĂẰẮẶẲẴĐÈÉẸẺẼÊỀẾỆỂỄÌÍỊỈĨÒÓỌỎÕÔỒỐỘỔỖƠỜỚỢỞỠÙÚỤỦŨƯỪỨỰỬỮỲÝỴỶỸ]+$/i,
'ku-IQ': /^[ئابپتجچحخدرڕزژسشعغفڤقکگلڵمنوۆھەیێيطؤثآإأكضصةظذ]+$/i,
'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can move this to L10 so that it's in alphabetic order.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing!,
ill do it tonight (got to pray first)

@profnandaa

Copy link
Copy Markdown
Member

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

Im sorry but i didnt get it, in which file should i see this ?

@profnandaaprofnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 7, 2020
@profnandaa

Copy link
Copy Markdown
Member

That's alpha.js; let me know if there's anything that should be done.

@fakhrip

fakhrip commented Oct 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we just add this to line 121 in alpha.js to kind of re-override it ?
alpha['fa-IR'] = alpha['fa-IR'];

@profnandaa

Copy link
Copy Markdown
Member

Ok, that works, you can drop in a comment before that.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Done, if there are anything i should do, please tell me 🙏

@profnandaaprofnandaa changed the title feat(isAlpha): feat(isAlpha) fix ح character validation in fa-IR language code (#1400)fix(isAlpha): fix ح character validation in fa-IR language code (#1400)Oct 9, 2020

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib once again! 🎉

@profnandaa
profnandaa merged commit 0c55656 into validatorjs:masterOct 9, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Sure, thank you so much for being good as this is my first legit PR 😃

@profnandaa

Copy link
Copy Markdown
Member

Welcome buddy, looking forward to seeing more from you :)

@tux-tn

Copy link
Copy Markdown
Member

@profnandaa@fakhrip i don't understand the purpose of the change in L124 😅

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

It was overriden by L101 @tux-tn ,
So there i have to "reoverride" it to set the fa-ir to fa-ir (it may sound confusing though, haha)

@tux-tn

Copy link
Copy Markdown
Member

I still don't get it, since alpha array has already been mutated alpha['fa-ir'] received alpha['fa'] content, how is re-overriding it is supposed to bring back the old content?
Logging content before and after the line shows the same value

@fakhrip

Copy link
Copy Markdown
ContributorAuthor
exportconstfarsiLocales=['IR','AF',];for(letlocale,i=0;i<farsiLocales.length;i++){locale=`fa-${farsiLocales[i]}`;alpha[locale]=alpha.fa;// This line changes alpha[fa-IR] content to alpha.faalphanumeric[locale]=alphanumeric.fa;decimal[locale]=decimal.fa;}

And because that the content of alpha.fa and alpha[fa-IR] is different (look below)

'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,fa: /^['0-9آاءأؤئبپتثجچحخدذرزژسشصضطظعغفقکگلمنوهةی۱۲۳۴۵۶۷۸۹۰']+$/i,

It will then be needed to re-override the alpha[fa-IR] that was changed to alpha.fa, to then become alpha[fa-IR] itself

@tux-tn

tux-tn commented Nov 11, 2020

Copy link
Copy Markdown
Member

alpha[locale] = alpha.fa;alpha[fa-IR] has been mutated and contains content of alpha.fa here, right?
Since `alpha[fa_IR] has already been mutated where is stored the old value that you need to reassign?

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Oh wait, i just realized that i was wrong XD, i'm very sorry dude, now i feel like really stupid...

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Do you think i need to fix this in another PR as this one already got merged, or what should i do best ?

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

Sure, you're statement is all good, it was my fault, i'm sorry @tux-tn

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip no problem my friend, thank you for taking the time to answer my questions. It's not a big deal

@profnandaa

Copy link
Copy Markdown
Member

thanks for the due diligence on this @tux-tn

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem in isAlpha( str ,['fa-IR']) not verify ح character

3 participants

@fakhrip@profnandaa@tux-tn
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(isAlpha): fix ح character validation in fa-IR language code (#1400) - #1455

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha
Oct 9, 2020
Merged

fix(isAlpha): fix ح character validation in fa-IR language code (#1400)#1455
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha

Conversation

@fakhrip

@fakhripfakhrip commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

I added some Persian alphabet characters retrieved from wikipedia on this line
Also add new validation regarding the language in the validator.js

This PR specifically fix#1400

Checklist

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

@codecov

codecovBot commented Oct 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1455 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1455 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 96 96 Lines 1275 1276 +1 =======================================
+ Hits 1274 1275 +1 
Misses 1 1 
Impacted FilesCoverage Δ
src/lib/alpha.js100.00% <100.00%> (ø)
src/lib/isPostalCode.js100.00% <0.00%> (ø)
src/lib/isMobilePhone.js100.00% <0.00%> (ø)
src/lib/isPassportNumber.js100.00% <0.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 7916dfd...a83805e. Read the comment docs.

@profnandaa

Copy link
Copy Markdown
Member

Thanks for your contribution! Please rebase your branch with master so as to remove the unrelated changes, then we can review.

@profnandaaprofnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 5, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

I have removed all unnecessary changes, please tell me if i do something wrong here.

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib 🎉 // Just update the README and we should be good to go.

@fakhrip

fakhrip commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Im pretty sure that there are nothing to be updated in the readme file regarding these changes as its already well-documented there (in the readme) @profnandaa

Edit: I've checked all the boxes so its all clear

Comment threadsrc/lib/alpha.js Outdated
'uk-UA': /^[А-ЩЬЮЯЄIЇҐі]+$/i,
'vi-VN': /^[A-ZÀÁẠẢÃÂẦẤẬẨẪĂẰẮẶẲẴĐÈÉẸẺẼÊỀẾỆỂỄÌÍỊỈĨÒÓỌỎÕÔỒỐỘỔỖƠỜỚỢỞỠÙÚỤỦŨƯỪỨỰỬỮỲÝỴỶỸ]+$/i,
'ku-IQ': /^[ئابپتجچحخدرڕزژسشعغفڤقکگلڵمنوۆھەیێيطؤثآإأكضصةظذ]+$/i,
'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can move this to L10 so that it's in alphabetic order.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing!,
ill do it tonight (got to pray first)

@profnandaa

Copy link
Copy Markdown
Member

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

Im sorry but i didnt get it, in which file should i see this ?

@profnandaaprofnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 7, 2020
@profnandaa

Copy link
Copy Markdown
Member

That's alpha.js; let me know if there's anything that should be done.

@fakhrip

fakhrip commented Oct 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we just add this to line 121 in alpha.js to kind of re-override it ?
alpha['fa-IR'] = alpha['fa-IR'];

@profnandaa

Copy link
Copy Markdown
Member

Ok, that works, you can drop in a comment before that.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Done, if there are anything i should do, please tell me 🙏

@profnandaaprofnandaa changed the title feat(isAlpha): feat(isAlpha) fix ح character validation in fa-IR language code (#1400)fix(isAlpha): fix ح character validation in fa-IR language code (#1400)Oct 9, 2020

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib once again! 🎉

@profnandaa
profnandaa merged commit 0c55656 into validatorjs:masterOct 9, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Sure, thank you so much for being good as this is my first legit PR 😃

@profnandaa

Copy link
Copy Markdown
Member

Welcome buddy, looking forward to seeing more from you :)

@tux-tn

Copy link
Copy Markdown
Member

@profnandaa@fakhrip i don't understand the purpose of the change in L124 😅

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

It was overriden by L101 @tux-tn ,
So there i have to "reoverride" it to set the fa-ir to fa-ir (it may sound confusing though, haha)

@tux-tn

Copy link
Copy Markdown
Member

I still don't get it, since alpha array has already been mutated alpha['fa-ir'] received alpha['fa'] content, how is re-overriding it is supposed to bring back the old content?
Logging content before and after the line shows the same value

@fakhrip

Copy link
Copy Markdown
ContributorAuthor
exportconstfarsiLocales=['IR','AF',];for(letlocale,i=0;i<farsiLocales.length;i++){locale=`fa-${farsiLocales[i]}`;alpha[locale]=alpha.fa;// This line changes alpha[fa-IR] content to alpha.faalphanumeric[locale]=alphanumeric.fa;decimal[locale]=decimal.fa;}

And because that the content of alpha.fa and alpha[fa-IR] is different (look below)

'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,fa: /^['0-9آاءأؤئبپتثجچحخدذرزژسشصضطظعغفقکگلمنوهةی۱۲۳۴۵۶۷۸۹۰']+$/i,

It will then be needed to re-override the alpha[fa-IR] that was changed to alpha.fa, to then become alpha[fa-IR] itself

@tux-tn

tux-tn commented Nov 11, 2020

Copy link
Copy Markdown
Member

alpha[locale] = alpha.fa;alpha[fa-IR] has been mutated and contains content of alpha.fa here, right?
Since `alpha[fa_IR] has already been mutated where is stored the old value that you need to reassign?

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Oh wait, i just realized that i was wrong XD, i'm very sorry dude, now i feel like really stupid...

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Do you think i need to fix this in another PR as this one already got merged, or what should i do best ?

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

Sure, you're statement is all good, it was my fault, i'm sorry @tux-tn

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip no problem my friend, thank you for taking the time to answer my questions. It's not a big deal

@profnandaa

Copy link
Copy Markdown
Member

thanks for the due diligence on this @tux-tn

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem in isAlpha( str ,['fa-IR']) not verify ح character

3 participants

@fakhrip@profnandaa@tux-tn
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(isAlpha): fix ح character validation in fa-IR language code (#1400) - #1455

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha
Oct 9, 2020
Merged

fix(isAlpha): fix ح character validation in fa-IR language code (#1400)#1455
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha

Conversation

@fakhrip

@fakhripfakhrip commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

I added some Persian alphabet characters retrieved from wikipedia on this line
Also add new validation regarding the language in the validator.js

This PR specifically fix#1400

Checklist

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

@codecov

codecovBot commented Oct 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1455 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1455 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 96 96 Lines 1275 1276 +1 =======================================
+ Hits 1274 1275 +1 
Misses 1 1 
Impacted FilesCoverage Δ
src/lib/alpha.js100.00% <100.00%> (ø)
src/lib/isPostalCode.js100.00% <0.00%> (ø)
src/lib/isMobilePhone.js100.00% <0.00%> (ø)
src/lib/isPassportNumber.js100.00% <0.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 7916dfd...a83805e. Read the comment docs.

@profnandaa

Copy link
Copy Markdown
Member

Thanks for your contribution! Please rebase your branch with master so as to remove the unrelated changes, then we can review.

@profnandaaprofnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 5, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

I have removed all unnecessary changes, please tell me if i do something wrong here.

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib 🎉 // Just update the README and we should be good to go.

@fakhrip

fakhrip commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Im pretty sure that there are nothing to be updated in the readme file regarding these changes as its already well-documented there (in the readme) @profnandaa

Edit: I've checked all the boxes so its all clear

Comment threadsrc/lib/alpha.js Outdated
'uk-UA': /^[А-ЩЬЮЯЄIЇҐі]+$/i,
'vi-VN': /^[A-ZÀÁẠẢÃÂẦẤẬẨẪĂẰẮẶẲẴĐÈÉẸẺẼÊỀẾỆỂỄÌÍỊỈĨÒÓỌỎÕÔỒỐỘỔỖƠỜỚỢỞỠÙÚỤỦŨƯỪỨỰỬỮỲÝỴỶỸ]+$/i,
'ku-IQ': /^[ئابپتجچحخدرڕزژسشعغفڤقکگلڵمنوۆھەیێيطؤثآإأكضصةظذ]+$/i,
'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can move this to L10 so that it's in alphabetic order.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing!,
ill do it tonight (got to pray first)

@profnandaa

Copy link
Copy Markdown
Member

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

Im sorry but i didnt get it, in which file should i see this ?

@profnandaaprofnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 7, 2020
@profnandaa

Copy link
Copy Markdown
Member

That's alpha.js; let me know if there's anything that should be done.

@fakhrip

fakhrip commented Oct 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we just add this to line 121 in alpha.js to kind of re-override it ?
alpha['fa-IR'] = alpha['fa-IR'];

@profnandaa

Copy link
Copy Markdown
Member

Ok, that works, you can drop in a comment before that.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Done, if there are anything i should do, please tell me 🙏

@profnandaaprofnandaa changed the title feat(isAlpha): feat(isAlpha) fix ح character validation in fa-IR language code (#1400)fix(isAlpha): fix ح character validation in fa-IR language code (#1400)Oct 9, 2020

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib once again! 🎉

@profnandaa
profnandaa merged commit 0c55656 into validatorjs:masterOct 9, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Sure, thank you so much for being good as this is my first legit PR 😃

@profnandaa

Copy link
Copy Markdown
Member

Welcome buddy, looking forward to seeing more from you :)

@tux-tn

Copy link
Copy Markdown
Member

@profnandaa@fakhrip i don't understand the purpose of the change in L124 😅

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

It was overriden by L101 @tux-tn ,
So there i have to "reoverride" it to set the fa-ir to fa-ir (it may sound confusing though, haha)

@tux-tn

Copy link
Copy Markdown
Member

I still don't get it, since alpha array has already been mutated alpha['fa-ir'] received alpha['fa'] content, how is re-overriding it is supposed to bring back the old content?
Logging content before and after the line shows the same value

@fakhrip

Copy link
Copy Markdown
ContributorAuthor
exportconstfarsiLocales=['IR','AF',];for(letlocale,i=0;i<farsiLocales.length;i++){locale=`fa-${farsiLocales[i]}`;alpha[locale]=alpha.fa;// This line changes alpha[fa-IR] content to alpha.faalphanumeric[locale]=alphanumeric.fa;decimal[locale]=decimal.fa;}

And because that the content of alpha.fa and alpha[fa-IR] is different (look below)

'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,fa: /^['0-9آاءأؤئبپتثجچحخدذرزژسشصضطظعغفقکگلمنوهةی۱۲۳۴۵۶۷۸۹۰']+$/i,

It will then be needed to re-override the alpha[fa-IR] that was changed to alpha.fa, to then become alpha[fa-IR] itself

@tux-tn

tux-tn commented Nov 11, 2020

Copy link
Copy Markdown
Member

alpha[locale] = alpha.fa;alpha[fa-IR] has been mutated and contains content of alpha.fa here, right?
Since `alpha[fa_IR] has already been mutated where is stored the old value that you need to reassign?

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Oh wait, i just realized that i was wrong XD, i'm very sorry dude, now i feel like really stupid...

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Do you think i need to fix this in another PR as this one already got merged, or what should i do best ?

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

Sure, you're statement is all good, it was my fault, i'm sorry @tux-tn

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip no problem my friend, thank you for taking the time to answer my questions. It's not a big deal

@profnandaa

Copy link
Copy Markdown
Member

thanks for the due diligence on this @tux-tn

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem in isAlpha( str ,['fa-IR']) not verify ح character

3 participants

@fakhrip@profnandaa@tux-tn
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(isAlpha): fix ح character validation in fa-IR language code (#1400) - #1455

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha
Oct 9, 2020
Merged

fix(isAlpha): fix ح character validation in fa-IR language code (#1400)#1455
profnandaa merged 3 commits into
validatorjs:masterfrom
fakhrip:fix-persian-alpha

Conversation

@fakhrip

@fakhripfakhrip commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

I added some Persian alphabet characters retrieved from wikipedia on this line
Also add new validation regarding the language in the validator.js

This PR specifically fix#1400

Checklist

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

@codecov

codecovBot commented Oct 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1455 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1455 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 96 96 Lines 1275 1276 +1 =======================================
+ Hits 1274 1275 +1 
Misses 1 1 
Impacted FilesCoverage Δ
src/lib/alpha.js100.00% <100.00%> (ø)
src/lib/isPostalCode.js100.00% <0.00%> (ø)
src/lib/isMobilePhone.js100.00% <0.00%> (ø)
src/lib/isPassportNumber.js100.00% <0.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 7916dfd...a83805e. Read the comment docs.

@profnandaa

Copy link
Copy Markdown
Member

Thanks for your contribution! Please rebase your branch with master so as to remove the unrelated changes, then we can review.

@profnandaaprofnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 5, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

I have removed all unnecessary changes, please tell me if i do something wrong here.

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib 🎉 // Just update the README and we should be good to go.

@fakhrip

fakhrip commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Im pretty sure that there are nothing to be updated in the readme file regarding these changes as its already well-documented there (in the readme) @profnandaa

Edit: I've checked all the boxes so its all clear

Comment threadsrc/lib/alpha.js Outdated
'uk-UA': /^[А-ЩЬЮЯЄIЇҐі]+$/i,
'vi-VN': /^[A-ZÀÁẠẢÃÂẦẤẬẨẪĂẰẮẶẲẴĐÈÉẸẺẼÊỀẾỆỂỄÌÍỊỈĨÒÓỌỎÕÔỒỐỘỔỖƠỜỚỢỞỠÙÚỤỦŨƯỪỨỰỬỮỲÝỴỶỸ]+$/i,
'ku-IQ': /^[ئابپتجچحخدرڕزژسشعغفڤقکگلڵمنوۆھەیێيطؤثآإأكضصةظذ]+$/i,
'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can move this to L10 so that it's in alphabetic order.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing!,
ill do it tonight (got to pray first)

@profnandaa

Copy link
Copy Markdown
Member

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip -- my bad, thought it was a new locale.
However, see what you need to fix in L94-103 since it will override what you have added.

Im sorry but i didnt get it, in which file should i see this ?

@profnandaaprofnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 7, 2020
@profnandaa

Copy link
Copy Markdown
Member

That's alpha.js; let me know if there's anything that should be done.

@fakhrip

fakhrip commented Oct 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we just add this to line 121 in alpha.js to kind of re-override it ?
alpha['fa-IR'] = alpha['fa-IR'];

@profnandaa

Copy link
Copy Markdown
Member

Ok, that works, you can drop in a comment before that.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Done, if there are anything i should do, please tell me 🙏

@profnandaaprofnandaa changed the title feat(isAlpha): feat(isAlpha) fix ح character validation in fa-IR language code (#1400)fix(isAlpha): fix ح character validation in fa-IR language code (#1400)Oct 9, 2020

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib once again! 🎉

@profnandaa
profnandaa merged commit 0c55656 into validatorjs:masterOct 9, 2020
@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Sure, thank you so much for being good as this is my first legit PR 😃

@profnandaa

Copy link
Copy Markdown
Member

Welcome buddy, looking forward to seeing more from you :)

@tux-tn

Copy link
Copy Markdown
Member

@profnandaa@fakhrip i don't understand the purpose of the change in L124 😅

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

It was overriden by L101 @tux-tn ,
So there i have to "reoverride" it to set the fa-ir to fa-ir (it may sound confusing though, haha)

@tux-tn

Copy link
Copy Markdown
Member

I still don't get it, since alpha array has already been mutated alpha['fa-ir'] received alpha['fa'] content, how is re-overriding it is supposed to bring back the old content?
Logging content before and after the line shows the same value

@fakhrip

Copy link
Copy Markdown
ContributorAuthor
exportconstfarsiLocales=['IR','AF',];for(letlocale,i=0;i<farsiLocales.length;i++){locale=`fa-${farsiLocales[i]}`;alpha[locale]=alpha.fa;// This line changes alpha[fa-IR] content to alpha.faalphanumeric[locale]=alphanumeric.fa;decimal[locale]=decimal.fa;}

And because that the content of alpha.fa and alpha[fa-IR] is different (look below)

'fa-IR': /^[ابپتثجچحخدذرزژسشصضطظعغفقکگلمنوهی]+$/i,fa: /^['0-9آاءأؤئبپتثجچحخدذرزژسشصضطظعغفقکگلمنوهةی۱۲۳۴۵۶۷۸۹۰']+$/i,

It will then be needed to re-override the alpha[fa-IR] that was changed to alpha.fa, to then become alpha[fa-IR] itself

@tux-tn

tux-tn commented Nov 11, 2020

Copy link
Copy Markdown
Member

alpha[locale] = alpha.fa;alpha[fa-IR] has been mutated and contains content of alpha.fa here, right?
Since `alpha[fa_IR] has already been mutated where is stored the old value that you need to reassign?

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Oh wait, i just realized that i was wrong XD, i'm very sorry dude, now i feel like really stupid...

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

Do you think i need to fix this in another PR as this one already got merged, or what should i do best ?

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

@fakhrip

Copy link
Copy Markdown
ContributorAuthor

@fakhrip actually i'm asking the question because i'm working on a PR to update dependencies (including eslint who keeps screaming about that line) i fixed it in my branch and i'll probably create a PR by the end of the week.

Sure, you're statement is all good, it was my fault, i'm sorry @tux-tn

@tux-tn

Copy link
Copy Markdown
Member

@fakhrip no problem my friend, thank you for taking the time to answer my questions. It's not a big deal

@profnandaa

Copy link
Copy Markdown
Member

thanks for the due diligence on this @tux-tn

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem in isAlpha( str ,['fa-IR']) not verify ح character

3 participants

@fakhrip@profnandaa@tux-tn