-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Refactor isTaxID for supporting more locales #1409
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
* Prefix US-related functionality with "enUs" * Remove assumption of global IRS-like campus prefixes * Move US prefix check into separate function * Add per-locale check function lookup table * Support future locales lacking a check function (e.g. UK) * Remove unneeded sorting of the IRS campus prefixes * Add comments
Codecov Report
@@ Coverage Diff @@
## master #1409 +/- ##
===========================================
- Coverage 100.00% 99.92% -0.08%
===========================================
Files 95 95
Lines 1248 1251 +3
===========================================
+ Hits 1248 1250 +2
- Misses 0 1 +1
Continue to review full report at Codecov.
|
|
Code coverage will increase once another locale is supported (and tested). However, I think that this should be a separate commit. |
profnandaa
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.
|
For more tests to be added I'll need to add support for another locale. I don't think that such a change should be part of this PR. One of my students has completed support for many more locales (including tests), and is ready to issue a PR once this gets accepted. |
|
@dspinellis what about we just add tests for the one locale/s currently supported/written here. |
|
The locale currently supported is already unit-tested; I wouldn't commit code without unit tests. |
|
Here is the corresponding test. |
Checklist