Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Intl] Cleanup#31366
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
[Intl] Cleanup#31366
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| 'XSU' =>true,// Sucre | ||
| 'XDR' =>true,// Special Drawing Rights | ||
| 'XTS' =>true,// Testing Currency Code | ||
| 'XXX' =>true,// Unknown Currency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure what's the point of this. Constants were self explanatory, now we need a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maintaining the consts is an extra task, not worth it IMHO. i didnt follow it for languages also.
feel free to ignore :) but it reduces noise and makes adding entries more straightforward (the consts are only used here; it's not real API needed)
| useSymfony\Component\Intl\Data\Bundle\Reader\BundleEntryReaderInterface; | ||
| useSymfony\Component\Intl\Exception\MissingResourceException; | ||
| useSymfony\Component\Intl\Locale; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Erm... are you sure about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yep :)
symfony/src/Symfony/Component/Intl/ResourceBundle/CurrencyBundleInterface.php
Lines 25 to 26 in70a941e
| * @param string $displayLocale Optional. The locale to return the result in | |
| * Defaults to {@link \Locale::getDefault()}. |
updated as such below
it's a tiny improvement, but worth clarifying IMHO to rely on the root\Locale (either the real one, or theIntl\Locale\Locale stub.
Intl\Locale extends from\Locale and doesnt own the default, only the default fallback.
This internal, first citizent,Intl\Locale is super confusing, and is really aLocaleFallback util
fabpot commentedMay 4, 2019
@jakzal Are you ok to merge this one? |
ro0NL commentedMay 6, 2019
the sole purpose of the PR is to reduce merge conflicts, if it doesnt matter much we can close (but in master /#31365 we take a different apprach, as im really skeptical about maintaining the lists of consts :D) |
fabpot commentedMay 7, 2019
Thank you@ro0NL. |
This PR was merged into the 3.4 branch.Discussion----------[Intl] Cleanup| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| 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 -->cleanup of#31365 for 3.4 + some other stuff to keep in sync across branchesCommits-------70a941e [Intl] Cleanup
Uh oh!
There was an error while loading.Please reload this page.
cleanup of#31365 for 3.4 + some other stuff to keep in sync across branches