-
-
Notifications
You must be signed in to change notification settings - Fork 974
fix(iban): more strict pattern for IE and PS #3464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3464 +/- ##
=======================================
Coverage 99.97% 99.97%
=======================================
Files 2819 2819
Lines 217500 217500
Branches 953 952 -1
=======================================
Hits 217447 217447
Misses 53 53
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes the used algorithm from type c to a which then does not generate digits anymore
faker/src/modules/finance/index.ts
Lines 921 to 929 in 048c325
if (bban.type === 'a') { | |
s += this.faker.helpers.arrayElement(iban.alpha); | |
} else if (bban.type === 'c') { | |
if (this.faker.datatype.boolean(0.8)) { | |
s += this.faker.number.int(9); | |
} else { | |
s += this.faker.helpers.arrayElement(iban.alpha); | |
} | |
} else { |
I guess you answered that because the implementation wasn't that clear? At least, that's what I was thinking when starting to work on the issue. I'm certain we could update this code (in a separate PR) to make it more readable. I'm also not sure if we need all featured that the Iban data structure provides 🤷 |
Yes, all good |
I agree the IBAN code is quite incomprehensible. Even some minor changes like defining a constant for "c" and "a" at top of the file would help readability |
I agree with both of you. I'll work on a second PR to make this more readable. I'm just wondering whether we should consider performance metrics here. While I could immediately think of multiple ways of how the implementation could be written more readable, all would definitely be slower (mostly due to multiple iterations over the expected pattern). |
I'm also most times in favor of readability and maintainability over performance. JS is not the language of choice when it comes to processing millions of data efficiently anyway. |
Description
This PR fixes #3463.
How to Test
To verify the change you can open the deploy review of the docs and past the following in the dev console:
The expected output is the log that tells you that all ibans were valid. If you want to repeat the test (with newly generated ibans) you can simply call
testRandomIban()
in the dev console again.