Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Intl] Simplify API#28846

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromro0NL:intl
Apr 15, 2019
Merged

[Intl] Simplify API#28846

fabpot merged 1 commit intosymfony:masterfromro0NL:intl
Apr 15, 2019

Conversation

@ro0NL
Copy link
Contributor

@ro0NLro0NL commentedOct 12, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#18368
LicenseMIT
Doc PRsymfony/symfony-docs#11221

Simplifies the Intl API. It greatly reduces the no. of boilerplate classes in this component. Very over complicated, much wow :)

Solving (IMHO):

class LanguageBundleextends LanguageDataProviderimplements LanguageBundleInterface

Which seems very over complicated just to provide static data.

// beforeIntl::getLanguageBundle()->getLanguageName()// string | null// afterLanguages::getName()// stringLanguages::exists()// bool

I left out Canonicalization on puropose, that's a new topic to me.

Thoughts?

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks, here are some random comments :)
Works for me generally speaking.
Would need some tests+UPGRADE/CHANGELOG entries

ro0NL reacted with thumbs up emoji
@ro0NL
Copy link
ContributorAuthor

i've moved all data bundles, ready for first round of review.

The newintl-data tests are passing locally 👍, i've copied fromSymfony\Component\Intl\Tests\Data\Provider so perhaps remove the legacy tests already? as they are somewhat big (but not sure we care).

Copy link
Contributor

@webmozartwebmozart left a comment

Choose a reason for hiding this comment

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

Great work@ro0NL :) Thanks a lot!

I'd finish this PR as is (it's pretty much done) and add your further work to separate PRs.

@ro0NL
Copy link
ContributorAuthor

@webmozart thanks for review, it's done :) i was a bit confused bylocales vs.currency codes (e.g. notlocale codes norcurrencies). But "code" is indeed part of their domain terminology, so is language code, script code etc. But not locales :)

Last but not least for local variables, e.g.$currency we mean the "code" and not the name or other. Works for me 👍

Ill fix the remaning deprecations asap.

@webmozart
Copy link
Contributor

@ro0NL Exactly! :) It takes some wrapping your mind around, but once you understand the domain language of CLDR, this naming makes total sense.

As for$currency et al., I'd even go as far as naming that$currencyCode (otherwise - is it the code? The name? ACurrency object?). But I know this may go a little far and am really fine with either way. :)

@ro0NLro0NL changed the title[Intl][WIP] Simplify API[Intl] Simplify APIApr 8, 2019
@ro0NL
Copy link
ContributorAuthor

status: ready :)

Copy link
Member

@javiereguiluzjaviereguiluz left a comment

Choose a reason for hiding this comment

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

I like this a lot! Thanks Roland.

@fabpot
Copy link
Member

Thank you@ro0NL.

ro0NL and andreybolonin reacted with hooray emoji

@fabpotfabpot merged commitd6b67d4 intosymfony:masterApr 15, 2019
fabpot added a commit that referenced this pull requestApr 15, 2019
This PR was squashed before being merged into the 4.3-dev branch (closes#28846).Discussion----------[Intl] Simplify API| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets |#18368| License       | MIT| Doc PR        |symfony/symfony-docs#11221Simplifies the Intl API. It greatly reduces the no. of boilerplate classes in this component. Very over complicated, much wow :)Solving (IMHO):```phpclass LanguageBundle extends LanguageDataProvider implements LanguageBundleInterface```Which seems very over complicated just to provide static data.```php// beforeIntl::getLanguageBundle()->getLanguageName() // string | null// afterLanguages::getName() // stringLanguages::exists() // bool```I left out Canonicalization on puropose, that's a new topic to me.- [x] Languages- [x] Locales- [x] Currencies- [x] Regions- [x] Scripts- [ ] Timezones (#28831)- [x] Update constraints- [x] Update form typesThoughts?Commits-------d6b67d4 [Intl] Simplify API
@ro0NLro0NL deleted the intl branchApril 15, 2019 12:50
fabpot added a commit that referenced this pull requestApr 29, 2019
This PR was merged into the 4.3-dev branch.Discussion----------[Intl] Fix LocaleDataGenerator| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->Forgotten in#28846The `getName()` method for scripts/regions/languages is stilled needed during locale generation.Commits-------137ce3f [Intl] Fix LocaleDataGenerator
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
OskarStark added a commit to symfony/symfony-docs that referenced this pull requestNov 29, 2019
…zki)This PR was submitted for the 5.0 branch but it was merged into the 4.3 branch instead (closes#12720).Discussion----------Changed names of Intl classes in form types docsAs persymfony/symfony#28846 we changed Intl classes to be more specific. Looks like some docs left with old naming.Commits-------f5f64e1 Changed names of Intl classes in form types docs
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof requested changes

@fabpotfabpotfabpot approved these changes

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuh

+1 more reviewer

@webmozartwebmozartwebmozart approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

7 participants

@ro0NL@webmozart@fabpot@javiereguiluz@nicolas-grekas@stof@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp