Skip to content

Add function twig *_name for intl#3742

Merged
fabpot merged 2 commits intotwigphp:3.xfrom
seb-jean:3.x
Dec 27, 2022
Merged

Add function twig *_name for intl#3742
fabpot merged 2 commits intotwigphp:3.xfrom
seb-jean:3.x

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

@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.

@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.

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.

5 participants

Comments