-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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]GSTIN Number Validation and PAN Validation (Multiple TIN functionality) and also refactored is taxID for options object #2153
Conversation
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #2153 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 106 106
Lines 2348 2362 +14
Branches 593 601 +8
=========================================
+ Hits 2348 2362 +14
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
I have a few remarks and suggestions. The added tests are nice, good work on those!
Shouldn't we be considering transitioning to an options object here (as per #1874)? Ensuring compatibility from now on should be fairly trivial to implement and introducing more convoluted options will make backwards compatibility more difficult to implement in the future. |
Updated readme for isTaxId regarding new localeOption functionality and options object.
src/lib/isTaxID.js
Outdated
function validateArgs(str, locale, options) { | ||
const localeOption = options?.localeOption; | ||
|
||
try { assertString(str); } catch (err) { throw new Error(`${err.message} for str`); } | ||
try { assertString(locale); } catch (err) { throw new Error(`${err.message} for locale`); } | ||
|
||
for (const option in options) { | ||
if (!(option in availableOptions)) throw new Error(`'${option}' is not available`); | ||
} | ||
|
||
if (!(locale in taxIdFormat)) throw new Error(`Invalid locale '${locale}'`); | ||
|
||
|
||
if (locale in multipleTinLocale) { | ||
try { assertString(localeOption); } catch (err) { throw new Error(`${err.message} for localeOption`); } | ||
if (!(localeOption in multipleTinLocale[locale])) throw new Error(`Invalid localeOption '${localeOption}'`); | ||
} else if (localeOption || localeOption === '') { | ||
throw new Error(`Invalid localeOption for locale '${locale}'`); | ||
} | ||
} | ||
|
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.
Did you get inspiration for this from some other validator using an options
object or did you come up with this yourself? To me this seems a bit heavy handed. Are we generally interested in throwing errors if the user doesn't provide valid options? I haven't seen this much argument validation in other validators. Does the multiple locale possibility in en-IN
warrant this much code here?
#2086 is somewhat similar to this PR in that it uses a local option in the options object. Maybe cut things down a bit, taking inspiration from there?
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.
Hey @braaar thanks for your feedback,
I came up with this approach of validating args, but took inspiration of options object from #2157 and #2075.
The current function also validates args and throws error if user specifies a wrong locale
or str
, so I thought it might be useful if we throw error for wrong options
too. And yeah since localeOption
is dependent on locale
it seems a bit heavy handed, even I had the same thought too. But it is just checking if the options provided are correct, if the locale has multiple TIN and also their types.
Wouldn't it be misleading if the validation returns false
when incorrect options are passed ?
Yeah #2086 is a bit similar but according to #1874 but locale
will not be a part of options object as it is mandatory. #1874 (comment).
For now, en-IN
is the only locale which has multiple TIN, but when I researched through the internet there are several other countries who have multiple TIN, may be this approach would help us to include other countries soon.
Example : US TIN
US has 5 different Identification numbers.
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 complexity is starting to make me feel like maintaining a validator for multiple tax ids for multiple countries is an enourmous undertaking. I suppose it's the only natural way forward, unless we decide to scrap the validator altogether, of course.
How many app developers are making international systems that care about tax ids and are using open source validation code? In my eyes the use case for such an all-encompassing validator is quite rare. When I work with things like social security numbers and business IDs I use a country-specific package that contains what I need and is much easier to maintain on its own, should I spot an error or want something added.
What are your thoughts on this, @WikiRik? I suppose this enters into a territory where the maintainers should chime in. They are in control of the main direction of the project and also feel the burden of maintaining code the most.
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.
Yes, input from the maintainers is indeed something we'll need. But the fact is that we already have isTaxID
with multiple countries so this is a logical addition in my view.
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.
@profnandaa can you please help us here.
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.
Perhaps I should open a separate issue about scrapping this validator.
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.
yeah and can we just add PAN regex alone for en-IN locale ,for now ?
Changed variable name to multipleTaxIdLocale Co-authored-by: Brage Sekse Aarset <[email protected]>
Added GSTIN Validation and PAN Validation for India TIN to isTaxID
GSTIN Format
PAN Format Refer PAN Format
Added a new
parameter locale option to the isTaxID functionoptions object , so users could now specify the required localeOption to select the required validator, if the locale has multiple TIN.Checklist