-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(finance): add IR iban #3678
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
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fakerjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @ds1371dani. Thanks for this contribution 🙌. We are handing development on the "next" branch. Please only provide PRs against it. I did go ahead and updated the target branch. This introduced conflicts in the package file, since the next branch is already using version 13.15.23 of validator. Would you be so kind and resolve the conflicts by merging "origin/next" into your branch or rebasing your branch onto "origin/next"? Also, I se that the snapshot tests are failing. Would you be so kind and resolve that issue as well? You can find infos about this problem in our contribution guide on our website. To resolve this concrete issue all you need to run is |
|
I worked though renovate requirements and let it create #3680 to update validator.js - that you don't need to resolve the conflicts and can simply revert the dependency update commit. |
0df7ff4 to
4995701
Compare
…cause of wrong IR iban validation)
4995701 to
f3145e0
Compare
|
Thanks @xDivisionByZerox for your quick answer, i rebased onto "origin/next" branch and also ran "preflight" script and rolled back validator package version I initially made my PR against v10 branch because i preferred this update to be included in the current major version. after merging into next branch when will it be released? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #3678 +/- ##
=======================================
Coverage 99.97% 99.97%
=======================================
Files 2995 2995
Lines 236313 236324 +11
Branches 940 939 -1
=======================================
+ Hits 236256 236267 +11
Misses 57 57
🚀 New features to boost your workflow:
|
This will happen with the next release. We rarely do backport, only for docs and security related issues. I sadly can't give you a more concrete day. Please remember that the entire Faker team is doing this project in their free time and likely wants to spend time with their family and friends just as anyone else. Thanks for understanding 🫶 |
xDivisionByZerox
left a comment
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.
Direct wikipeda link for other reviewers to verify the schema:
- https://en.wikipedia.org/wiki/International_Bank_Account_Number#IBAN_formats_by_country (under the first table in the section "Aspirational country codes for International Bank Account Number")
| expect( | ||
| 26, | ||
| `IR IBAN would be 26 chars length, given is ${iban.length}` | ||
| ).toBe(iban.length); | ||
|
|
||
| expect( | ||
| 32, | ||
| `IR formatted IBAN would be 32 chars length, given is ${ibanFormated.length}` | ||
| ).toBe(ibanFormated.length); |
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.
For other reviewer:
Usually I would suggest the following here:
By using the
toHaveLength()matcher, you do not need to provide an additional error message, since the matcher already take care of that. This would also align more closely with other test setups."Suggested change
expect( 26, `IR IBAN would be 26 chars length, given is ${iban.length}` ).toBe(iban.length); expect( 32, `IR formatted IBAN would be 32 chars length, given is ${ibanFormated.length}` ).toBe(ibanFormated.length); expect(iban).toHaveLength(26); expect(ibanFormated).toHaveLength(32);
But looking at the other test implementations for iban, this is not the case. So we should stay consistent with the other test in this file for now.
We should probably update the test file to use more self descripting matchers in a separate PR. By doing this we benefit from better, "native" error messages and a more coherent usage of matchers across all test files.
Currently the finance > iban is missing IR locale
after i added the patterns, my test didn't pass because of validator.js isIBAN validation (it had wrong pattern)
validator.js was fixed in this PR and so the validator package in faker was also updated