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

Add exceptions rates support #61

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add exceptions rates support #61

wants to merge 8 commits into from

Conversation

trippo
Copy link

@trippo trippo commented Sep 21, 2023

No description provided.

@VincentLanglet
Copy link
Contributor

Hi @trippo I would recommend to split the PR, you're trying to do 2 things here:

  • Adding exceptions rates supports
  • Removing the GB VAT number validations.

The second one is a breaking change

@trippo
Copy link
Author

trippo commented Nov 14, 2023

Re-added GB validator

@trippo
Copy link
Author

trippo commented Nov 15, 2023

@VincentLanglet please review

@VincentLanglet
Copy link
Contributor

@VincentLanglet please review

I'm not the maintainer, I cannot

  • launch github actions
  • approve
  • merge

@VincentLanglet
Copy link
Contributor

(But I would recommend you to open two separate PR: One with 84d6002 and one with e5014d7)

@@ -258,6 +258,7 @@ class Countries implements \Iterator, \ArrayAccess
'VI' => 'Virgin Islands, U.S.',
'WF' => 'Wallis And Futuna',
'EH' => 'Western Sahara',
'XI' => 'Northern Ireland',
Copy link
Contributor

Choose a reason for hiding this comment

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

Xi is not a valid IsoCode cf https://en.wikipedia.org/wiki/List_of_ISO_3166_country_codes

It's only used for VAT but it's not an isocode.

@@ -278,7 +279,7 @@ public function hasCountryCode(string $code) : bool
*/
public function isCountryCodeInEU(string $code) : bool
{
$eu = ['AT', 'BE', 'BG', 'CY', 'CZ', 'DE', 'DK', 'EE', 'ES', 'FI', 'FR', 'GB', 'GR', 'HU', 'HR', 'IE', 'IT', 'LT', 'LU', 'LV', 'MT', 'NL', 'PL', 'PT', 'RO', 'SE', 'SI', 'SK'];
$eu = ['AT', 'BE', 'BG', 'CY', 'CZ', 'DE', 'DK', 'EE', 'ES', 'FI', 'FR', 'GR', 'HU', 'HR', 'IE', 'IT', 'LT', 'LU', 'LV', 'MT', 'NL', 'PL', 'PT', 'RO', 'SE', 'SI', 'SK', 'XI'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, XI is not a valid isocode

@dannyvankooten
Copy link
Member

Hello @trippo,

I'm the maintainer yet I wholeheartedly agree with @VincentLanglet - unfortunately there are so many changes in this PR targeting several separate things that it's really hard for me to confirm what is changing and whether this is safe to merge in the codebase.

Separate PR's would be super helpful in making this a little easier for me.

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.

3 participants