Skip to content

Fix A-z ranges #1625

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.

Sign up for GitHub

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

Merged
profnandaa merged 5 commits into from
Apr 17, 2021
Merged

Fix A-z ranges #1625

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cfb6be2
Use correct BIC regex
bmacnaughton Mar 10, 2021
c1e8b23
fix isLocale alpha ranges
bmacnaughton Mar 10, 2021
369d1e9
Fix IE alpha range
bmacnaughton Mar 10, 2021
2c41f65
allow lowercase alpha
bmacnaughton Mar 10, 2021
0db8241
check country code too
bmacnaughton Mar 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/lib/isBIC.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import assertString from './util/assertString';

const isBICReg = /^[A-z]{4}[A-z]{2}\w{2}(\w{3})?$/;
// https://en.wikipedia.org/wiki/ISO_9362
const isBICReg = /^[A-Za-z]{6}[A-Za-z0-9]{2}([A-Za-z0-9]{3})?$/;
Copy link
Member

@ezkemboi ezkemboi Mar 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

NOTE: This is not part of the changes request. I am just offering suggestions and we can do it in a separate PR. I can also work on it after this PR get merged.

Great work here.
I like the simplicity, but I could offer an alternative based on fact that the second part is country code addition i.e:

  • Bank code (A-Z) : 4 letter code.

Country code (A-Z) : 2 letter code.

  • Location Code (0-9 or A-Z) : 2 digit code – either letters or numbers.
  • Branch Code (0-9 or A-Z) : optional 3 digit code – either letters or numbers*.

-> We could leverage the array in validISO31661Alpha2CountriesCodes for making this a robust validator.

NB: [Editing code based on comments]

// Making validISO31661Alpha2CountriesCodes exportable or just add to new util file
import { validISO31661Alpha2CountriesCodes } from './isISO31661Alpha2';
//... other code
export default function isBIC(str) {
  assertString(str);
  const countryCode = str.slice(4, 6);
  if(
    validISO31661Alpha2CountriesCodes.indexOf(countryCode) === -1 || 
    countryCode.toUpperCase !== 'XK' // for Republic of Kosovo
   ) {
    return false;
  }
  return isBICReg.test(str);
}

CC. @tux-tn and @profnandaa

Copy link
Contributor Author

@bmacnaughton bmacnaughton Mar 22, 2021

Choose a reason for hiding this comment

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

i'm happy to make a change doing this - i think it's a good idea. i would just return false without executing the isBICReg.test(str) if it's not a country code - no reason to execute the test if we already know the answer.

it's not clear to me why using includes is useful though; is there a compatibility issue somewhere that i'm not aware of? this seems like it should be a straight, simple use of indexOf() - some has to call a function each time but even then why replace some() with includes() to does the exact same thing but with an extra layer of a function call.

Copy link
Member

@ezkemboi ezkemboi Mar 22, 2021

Choose a reason for hiding this comment

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

That is great. And I am happy that you are willing to make these changes.

First, I would like to say that, the code above was a sample and required more modifications.

  1. If it not country code, please do that return as you have mentioned
  2. Take the array of validISO31661Alpha2CountriesCodes from isISO31661Alpha2 and create a new file inside lib/util. Import for both isISO31661Alpha2 and BIC validators.
  3. You can make use of indexOf() instead of includes() and some()

I am happy to review that and give a go-ahead to merge this PR.
Thanks again for your contributions @bmacnaughton.

Copy link
Contributor Author

@bmacnaughton bmacnaughton Mar 22, 2021

Choose a reason for hiding this comment

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

i think this is ready to be merged. it made sense to check that the bank ID was valid.


export default function isBIC(str) {
assertString(str);
Expand Down
2 changes: 1 addition & 1 deletion src/lib/isLocale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import assertString from './util/assertString';

const localeReg = /^[A-z]{2,4}([_-]([A-z]{4}|[\d]{3}))?([_-]([A-z]{2}|[\d]{3}))?$/;
const localeReg = /^[A-Za-z]{2,4}([_-]([A-Za-z]{4}|[\d]{3}))?([_-]([A-Za-z]{2}|[\d]{3}))?$/;

export default function isLocale(str) {
assertString(str);
Expand Down
2 changes: 1 addition & 1 deletion src/lib/isPostalCode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const patterns = {
HT: /^HT\d{4}$/,
HU: fourDigit,
ID: fiveDigit,
IE: /^(?!.*(?:o))[A-z]\d[\dw]\s\w{4}$/i,
IE: /^(?!.*(?:o))[A-Za-z]\d[\dw]\s\w{4}$/i,
IL: /^(\d{5}|\d{7})$/,
IN: /^((?!10|29|35|54|55|65|66|86|87|88|89)[1-9][0-9]{5})$/,
IR: /\b(?!(\d)\1{3})[13-9]{4}[1346-9][013-9]{5}\b/,
Expand Down