Skip to content
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

feat(niceToHave): Export list of country codes that implement IBAN #1669

Conversation

drorheller
Copy link
Contributor

@drorheller drorheller commented May 5, 2021

Export a list of country codes implementing IBAN
Addressing issue #1570

Checklist

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

@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #1669 (7123c94) into master (d1a9b6d) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 7123c94 differs from pull request most recent head 6006307. Consider uploading reports for the commit 6006307 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1669   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          101       101           
  Lines         1854      1855    +1     
=========================================
+ Hits          1854      1855    +1     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/lib/isIBAN.js 100.00% <100.00%> (ø)
src/lib/isPassportNumber.js 100.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 d1a9b6d...6006307. Read the comment docs.

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated
@@ -48,7 +48,7 @@ import isHSL from './lib/isHSL';

import isISRC from './lib/isISRC';

import isIBAN from './lib/isIBAN';
import isIBAN, { locales as isIBANLocales } from './lib/isIBAN';
Copy link
Member

Choose a reason for hiding this comment

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

May be calling them isIBANLocales could be a little misleading, might think it's a validator/function. Perhaps just drop the is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@profnandaa I see what you're saying, but I think it's more inline with the rest of the code to call it isIBANLocales

@profnandaa
Copy link
Member

Check also the failing tests.

@drorheller
Copy link
Contributor Author

Check also the failing tests.

@profnandaa pushed test fixes, could you please approve running the workflow?

@drorheller
Copy link
Contributor Author

Hi @fedeci :)
Can you approve the latest changes here?

fedeci
fedeci previously approved these changes Jul 11, 2021
Copy link
Contributor

@fedeci fedeci left a comment

Choose a reason for hiding this comment

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

LGTM, but I also agree with this comment

@@ -5,6 +5,7 @@ import { locales as isAlphaLocales } from '../src/lib/isAlpha';
import { locales as isAlphanumericLocales } from '../src/lib/isAlphanumeric';
import { locales as isMobilePhoneLocales } from '../src/lib/isMobilePhone';
import { locales as isFloatLocales } from '../src/lib/isFloat';
import { locales as ibanCountryCodes } from '../src/lib/isIBAN';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { locales as ibanCountryCodes } from '../src/lib/isIBAN';
import { locales as isIBANLocales } from '../src/lib/isIBAN';

I would name it isIBANLocales to align to the other one.

@drorheller
Copy link
Contributor Author

@fedeci got you, just made the change

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution.

@profnandaa profnandaa merged commit 2595554 into validatorjs:master Jul 16, 2021
@drorheller
Copy link
Contributor Author

LGTM, thanks for your contribution.

NP! thanks for your help :)

Sign up for free to 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.

4 participants