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

[Intl] Rename Regions to Countries #31350

Merged
merged 1 commit into from May 6, 2019

Conversation

Projects
None yet
9 participants
@ro0NL
Copy link
Contributor

commented May 1, 2019

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes (including intl-data group)
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

Because that's what the current region data is about; country codes.

This makes things consistent across the board; i.e. CountryType, CountryValidator

This allows a possible other region subset (e.g "continents") to distinct. Thus having Countries::class + Continents::class both reading from the region data. By then data should be compiled under different keys.

Current class is master only, so now or never :)

The alternative approach would be Regions::getCountryNames [,getContinentNames, etc.], which is harder to scale, and should also be decided for 4.3 ideally.

@ro0NL ro0NL force-pushed the ro0NL:countries branch 2 times, most recently from 0ed82d3 to 1c070fd May 1, 2019

@ro0NL ro0NL force-pushed the ro0NL:countries branch from 1c070fd to 49aee67 May 1, 2019

@stof

This comment has been minimized.

Copy link
Member

commented May 2, 2019

The region codes XA and XB in our region bundle don't look like countries. We should check them for mistakes.

But otherwise, I agree about this renaming, as this class deals with a subset of ICU regions, not with all regions, and so is a different concept.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

You're right, i suggest to consistently ignore all "non" valid codes, as done in #28833 for languages

See also http://unicode.org/reports/tr35/#Unknown_or_Invalid_Identifiers (we almost conform)

Do we agree excluding data is a new feature still?

@fabpot

fabpot approved these changes May 6, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Thank you @ro0NL.

@fabpot fabpot merged commit 49aee67 into symfony:master May 6, 2019

2 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request May 6, 2019

feature #31350 [Intl] Rename Regions to Countries (ro0NL)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Intl] Rename Regions to Countries

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes  (including intl-data group)
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Because that's what the current region data is about; country codes.

This makes things consistent across the board; i.e. CountryType, CountryValidator

This allows a possible other region subset (e.g "continents") to distinct. Thus having `Countries::class` + `Continents::class` both reading from the `region` data. By then data should be compiled under different keys.

Current class is master only, so now or never :)

The alternative approach would be `Regions::getCountryNames [,getContinentNames, etc.]`, which is harder to scale, and should also be decided for 4.3 ideally.

Commits
-------

49aee67 [Intl] Rename Regions to Countries
@javiereguiluz

This comment has been minimized.

Copy link
Member

commented May 6, 2019

I think we should revert this change. Regions are not the same as countries. There are lots of territorial disputes in the world which (in some cases) makes it impossible to define what a country is.

Even ICU itself declares that:

[Regions are ...] typically codes for countries, but also include areas that are not separate countries, such as the code "AQ" for Antarctica or the code "HK" for Hong Kong (SAR China). Overseas dependencies of countries may or may not have separate codes. The codes are typically 2-letter codes, but BCP47 allows for the use of 3-digit codes in the future.

Also, all the smart developers out there in all programming languages use "Region". You can also see that in all operating systems. Even when displaying it for end-users, they choose "region" over "country":

ios-region

mac-region

windows-region

@ro0NL ro0NL deleted the ro0NL:countries branch May 6, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@javiereguiluz we use the Regions api for CountryValidator and CountrType, that's inconsistent naming at least. This fixes that inconsistency.

AFAIK the current data are true "countries", as we filter upstream ICU data. As such; did you find a case where a "2 letter region code" is NOT an expected country code?

if so it should be patched IMHO (exclude those), see e.g. #31365

in general i think it's valid to provide a subset of regions; thus countries only.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@ro0NL yes, I've found lots of errors :(

There are many other mistakes. The only reasonable way to fix all this is ... reverting this PR and using "Region" instead of "Country". That's what everybody else does. Please, consider a revert 🙏 Thanks!

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

That's what everybody else does.

from a form perspective, not by Symfony: CountryType

Is it a RegionType actually? im really tempted to double check the list and exclude invalid countries such as Antarctica, etc. :/

but then again, renaming everything to Region is also an option. But it's a bigger step for the codebase in terms of deprecation etc.

And if we stick with "regions", does https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Intl/Data/Generator/RegionDataGenerator.php#L43 still apply? Technically we dont care anymore 🤔

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

i think we should filter by https://en.wikipedia.org/wiki/List_of_ISO_3166_country_codes

which states AQ (Antarctica) is an official (perhaps special) ISO country code

The ISO 3166 country name Antarctica comprises the continent of Antarctica and all land and ice shelves south of the 60th parallel south.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

TW (Taiwan) is disputed following ISO-3316, not sure that makes it an invalid country. I would keep it for now.

So following ISO-3316 seems like a good compromise?

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

currently Symfony provides 255 country codes (intl-data/region/en.json), with #31365 that is 252 even.

removing the aliases from https://en.wikipedia.org/wiki/List_of_ISO_3166_country_codes gives us 249 codes.

I think we're very close already, and probably if we remove the remaining "islands" and "territories" we should be good.

@fabpot

This comment has been minimized.

Copy link
Member

commented May 6, 2019

But then, it becomes our responsibility to filter regions, which is probably something we want to do in the long run. Woudn't it "better" to rename CountryValidator instead?

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

which is probably something we want to do in the long run

want or "dont want"? We already filter "some" regions today, but now im unsure what the intend is :) "exclude invalid regions" (what's an invalid region?) vs. "exclude invalid countries" (we know that's ISO-3166).

If we dont filter "regions" at all the list effectively becomes https://github.com/unicode-org/icu/blob/master/icu4c/source/data/region/en.txt ... so 001 (World) is a valid region?

from a domain perspective i value "countries" more than "regions", thus ISO-3166.

@fabpot

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Sorry, I means "don't want" here :)

I do understand that we need to filter some entries that are not countries (World). But filtering countries is way more complex as Javier noted. I think it's not Symfony's responsibility to make choices here. So, sticking to ISO-3166 is an option, keeping regions is another one. I must admit that I don't have any preference here but I recognize that we need to make an informed decision here.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I'd vote for sticking with ISO-3166 for now, thus "countries" and filter what's not an ISO-3166 code

see https://www.diffchecker.com/OJeLGEYU it's doable.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

the process is also easy IMHO, whenever we apply an ICU update we should verify only ISO-3166 country codes are added

@fabpot

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@symfony/deciders We need your point of view here. Thank you.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

in #31365 i've made the list iso compliant for comparison

if anything ... i'd say it was really close to that already, compared to "includes any region" :D

also there's #18613 which truly ask for an ISO list of countries

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 6, 2019

I'm for "Country" - the examples you gave Javier relate all to some geographical configuration. But if I want to create an "Address" field, I really want a "Country" label before entering "France". I think that's what this is for most of the time.

@Tobion

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Sticking to ISO-3166 seems fine and then it's technically countries. If someone wants to use a more generic term like "region" they can just change the label. But for most cases it's for adress fields as Nicolas said, and there only countries make sense because you cannot send a postcard to the EU or EZ. Or can you? Would be funny to try out.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

TLDR;

From https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2#Decoding_table

  • ICU excludes "Deleted", "Unassigned", etc. already
  • SF additionally should exclude "User-assigned" and "Exceptionally reserved"

After #31365 SF will provide the "officially assigned ISO 3166-1 alpha-2 codes"; those we call "Countries", hence this PR.

Thus, per ISO standards; e.g. Antarctica and Hong Honk are countries. Whether we can send a postcard to some address here is vague. For Hong Kong i believe yes, for Antarctica i believe not (yet there's a post office 😂 https://theplanetd.com/a-post-office-in-antarctica-you-betcha/)

Im not aware of why Antarctica (a continent) is assigned an official country code, i guess it's some practical reason (as it's a continent without countries).

fabpot added a commit that referenced this pull request May 7, 2019

feature #31365 [Intl] Made countries ISO 3166 compliant + exclude Zzz…
…z script code (ro0NL)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Intl] Made countries ISO 3166 compliant + exclude Zzzz script code

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes (including intl-data group)
| Fixed tickets | #31350 (comment), #18613
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

For consistency :)

Commits
-------

1bb7dde [Intl] Made countries ISO 3166 compliant + exclude Zzzz script code
@lyrixx

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Im not aware of why Antarctica (a continent) is assigned an official country code, i guess it's some practical reason (as it's a continent without countries).

Actually, Antartica is split in pieces that rattached to existing countries: https://en.wikipedia.org/wiki/Antarctica#Antarctic_territories

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.