-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Strict mode: Fix readme.md and fix getTIN function #43
Conversation
} | ||
|
||
return ''; | ||
return strtoupper($this->tin); |
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 would prefer to keep that in the existing normalize function. WDYT?
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 agree you
src/TIN.php
Outdated
@@ -120,6 +120,17 @@ public function isValid(bool $strict = false): bool | |||
return true; | |||
} | |||
|
|||
public function hasAlgorithm(): bool |
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.
How about keeping the "algorithm" side of things internally and expose a simpler interface?
In this case, I would just name this method isSupported()
? WDYT ?
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, this is better, I agree you, better isSupported()
Can you please resubmit your PR using a different branch, you should not use |
Hi, I have updated readme.md file and also fix getTIN function to return only upper version of TIN.
Before:
After:
Otherwise, validate function using this
$tin = $this->getTIN();
at the beginning is not using the correct stricted version of TIN.The place in where TIN is "normalized" is in your new parse function, so there is not necessary to normalized it again in getTIN funcion.
NOTE: if you want, you can even publish getTIN() as this:
but I think that strtoupper function doesnt hurt.