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 function twig *_name for intl #3742

Merged
merged 2 commits into from Dec 27, 2022
Merged

Add function twig *_name for intl #3742

merged 2 commits into from Dec 27, 2022

Conversation

seb-jean
Copy link
Contributor

Hi,

I add language_names, script_names, country_names, locale_names, currency_names, timezone_names.

It allows us to have a list but on the Twig side and to use them in filters for example.

@seb-jean
Copy link
Contributor Author

issue #3741

extra/intl-extra/IntlExtension.php Outdated Show resolved Hide resolved
@fabpot
Copy link
Contributor

fabpot commented Sep 30, 2022

Can you add the related docs?

@seb-jean
Copy link
Contributor Author

Can you add the related docs?

I just added them.

@SpacePossum
Copy link
Contributor

maybe some utest (coverage) could be added for the new code?

@seb-jean
Copy link
Contributor Author

maybe some utest (coverage) could be added for the new code?

I don't know how to do it.

@GromNaN
Copy link
Contributor

GromNaN commented Oct 2, 2022

maybe some utest (coverage) could be added for the new code?

I don't know how to do it.

You can add test files in extra/intl-extra/Tests/Fixtures and run the test command:

cd extra/intl-extra
composer install
vendor/bin/simple-phpunit

@seb-jean
Copy link
Contributor Author

seb-jean commented Oct 3, 2022

maybe some utest (coverage) could be added for the new code?

I don't know how to do it.

You can add test files in extra/intl-extra/Tests/Fixtures and run the test command:

cd extra/intl-extra
composer install
vendor/bin/simple-phpunit

I just added them.

Copy link
Contributor

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Good, the tests are exhaustive.
There is some words to replace in the doc.
The fabbot issue is wrong.

doc/functions/currency_names.rst Outdated Show resolved Hide resolved
doc/functions/locale_names.rst Outdated Show resolved Hide resolved
doc/functions/timezone_names.rst Outdated Show resolved Hide resolved
@fabpot
Copy link
Contributor

fabpot commented Oct 4, 2022

It looks like tests are broken.

@seb-jean
Copy link
Contributor Author

seb-jean commented Oct 4, 2022

The fabbot issue is wrong.

You are right but I did not touch this part of the code.
fabpot.io offers me the following code:

     public function formatDateTime(Environment $env, $date, ?string $dateFormat = 'medium', ?string $timeFormat = 'medium', string $pattern = '', $timezone = null, string $calendar = 'gregorian', string $locale = null): string
     {
-        $date = \twig_date_converter($env, $date, $timezone);
+        $date = twig_date_converter($env, $date, $timezone);
         $formatter = $this->createDateFormatter($locale, $dateFormat, $timeFormat, $pattern, $date->getTimezone(), $calendar);
 
         if (false === $ret = $formatter->format($date)) 

Otherwise, this is the link: https://fabbot.io/patch/twigphp/Twig/3742/cf584f042bf16324db8b8a7a74e067d2e41f03e8/cs.diff

@seb-jean
Copy link
Contributor Author

seb-jean commented Oct 4, 2022

It looks like tests are broken.

Tests fail due to line break (\r, \r\n)
I don't see how I could do better to pass the tests.

Copy link
Contributor

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I made some minor comments.

doc/functions/country_names.rst Outdated Show resolved Hide resolved
doc/functions/country_names.rst Show resolved Hide resolved
return [];
--EXPECT--
0
Afghanistan, Åland Islands, Albania, Algeria, American Samoa, Andorra, Angola, Anguilla, Antarctica, Antigua & Barbuda, Argentina, Armenia, Aruba, Australia, Austria, Azerbaijan, Bahamas, Bahrain, Bangladesh, Barbados, Belarus, Belgium, Belize, Benin, Bermuda, Bhutan, Bolivia, Bosnia & Herzegovina, Botswana, Bouvet Island, Brazil, British Indian Ocean Territory, British Virgin Islands, Brunei, Bulgaria, Burkina Faso, Burundi, Cambodia, Cameroon, Canada, Cape Verde, Caribbean Netherlands, Cayman Islands, Central African Republic, Chad, Chile, China, Christmas Island, Cocos (Keeling) Islands, Colombia, Comoros, Congo - Brazzaville, Congo - Kinshasa, Cook Islands, Costa Rica, Côte d’Ivoire, Croatia, Cuba, Curaçao, Cyprus, Czechia, Denmark, Djibouti, Dominica, Dominican Republic, Ecuador, Egypt, El Salvador, Equatorial Guinea, Eritrea, Estonia, Eswatini, Ethiopia, Falkland Islands, Faroe Islands, Fiji, Finland, France, French Guiana, French Polynesia, French Southern Territories, Gabon, Gambia, Georgia, Germany, Ghana, Gibraltar, Greece, Greenland, Grenada, Guadeloupe, Guam, Guatemala, Guernsey, Guinea, Guinea-Bissau, Guyana, Haiti, Heard & McDonald Islands, Honduras, Hong Kong SAR China, Hungary, Iceland, India, Indonesia, Iran, Iraq, Ireland, Isle of Man, Israel, Italy, Jamaica, Japan, Jersey, Jordan, Kazakhstan, Kenya, Kiribati, Kuwait, Kyrgyzstan, Laos, Latvia, Lebanon, Lesotho, Liberia, Libya, Liechtenstein, Lithuania, Luxembourg, Macao SAR China, Madagascar, Malawi, Malaysia, Maldives, Mali, Malta, Marshall Islands, Martinique, Mauritania, Mauritius, Mayotte, Mexico, Micronesia, Moldova, Monaco, Mongolia, Montenegro, Montserrat, Morocco, Mozambique, Myanmar (Burma), Namibia, Nauru, Nepal, Netherlands, New Caledonia, New Zealand, Nicaragua, Niger, Nigeria, Niue, Norfolk Island, North Korea, North Macedonia, Northern Mariana Islands, Norway, Oman, Pakistan, Palau, Palestinian Territories, Panama, Papua New Guinea, Paraguay, Peru, Philippines, Pitcairn Islands, Poland, Portugal, Puerto Rico, Qatar, Réunion, Romania, Russia, Rwanda, Samoa, San Marino, São Tomé & Príncipe, Saudi Arabia, Senegal, Serbia, Seychelles, Sierra Leone, Singapore, Sint Maarten, Slovakia, Slovenia, Solomon Islands, Somalia, South Africa, South Georgia & South Sandwich Islands, South Korea, South Sudan, Spain, Sri Lanka, St. Barthélemy, St. Helena, St. Kitts & Nevis, St. Lucia, St. Martin, St. Pierre & Miquelon, St. Vincent & Grenadines, Sudan, Suriname, Svalbard & Jan Mayen, Sweden, Switzerland, Syria, Taiwan, Tajikistan, Tanzania, Thailand, Timor-Leste, Togo, Tokelau, Tonga, Trinidad & Tobago, Tunisia, Turkey, Turkmenistan, Turks & Caicos Islands, Tuvalu, U.S. Outlying Islands, U.S. Virgin Islands, Uganda, Ukraine, United Arab Emirates, United Kingdom, United States, Uruguay, Uzbekistan, Vanuatu, Vatican City, Venezuela, Vietnam, Wallis & Futuna, Western Sahara, Yemen, Zambia, Zimbabwe
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests are going to break every time there is an update in the underlying dataset. I would suggest only listing some "stable" entries instead.

@fabpot
Copy link
Contributor

fabpot commented Dec 27, 2022

@seb-jean Thank you fo all the changes. I'm going to take over the tests changes before merging the PR (I'd like to have it for the next versions that I'm going to release soon).

@fabpot
Copy link
Contributor

fabpot commented Dec 27, 2022

Thank you @seb-jean.

@fabpot fabpot merged commit 3b46c89 into twigphp:3.x Dec 27, 2022
@seb-jean
Copy link
Contributor Author

Thanks to the people who helped me build this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants