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

Strict mode: Fix readme.md and fix getTIN function #43

Closed
wants to merge 5 commits into from

Conversation

ortegafernando
Copy link
Contributor

@ortegafernando ortegafernando commented Jun 8, 2024

Hi, I have updated readme.md file and also fix getTIN function to return only upper version of TIN.

Before:

    public function getTIN(): string
    {
        if (null !== $string = preg_replace('#[^[:alnum:]\-+]#u', '', $this->tin)) {
            return strtoupper($string);
        }
        return '';
    }

After:

    public function getTIN(): string
    {
        return strtoupper($this->tin);
    }

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:

    public function getTIN(): string
    {
        return $this->tin;
    }

but I think that strtoupper function doesnt hurt.

@ortegafernando ortegafernando requested a review from lekoala as a code owner June 8, 2024 06:17
}

return '';
return strtoupper($this->tin);
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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 ?

Copy link
Contributor Author

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()

@drupol
Copy link
Contributor

drupol commented Jul 17, 2024

Can you please resubmit your PR using a different branch, you should not use master but create your own branch.

@drupol drupol closed this Jul 17, 2024
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.

2 participants