-
Notifications
You must be signed in to change notification settings - Fork 966
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
Change BrazilianPortugueseOrdinalizer to PortugueseOrdinalizer #416
Conversation
Change BrazilianPortugueseOrdinalizer to PortugueseOrdinalizer because the rules are the same and the language is Portuguese and not Brazilian-Portuguese
@TiagoBrito thanks for the PR. Can you please add it to the |
@mexx Yes I can, but should I create a tag header: v1.35.1 - 2015-05-12 before the tag is created?? Isn't that weird? |
@TiagoBrito no, you put it under "In Development" header, but above "commits" |
@hazzik done, thanks for your feedback |
@@ -10,9 +10,9 @@ public OrdinalizerRegistry() : base(new DefaultOrdinalizer()) | |||
Register("en", new EnglishOrdinalizer()); | |||
Register("es", new SpanishOrdinalizer()); | |||
Register("it", new ItalianOrdinalizer()); | |||
Register("pt-BR", new BrazilianPortugueseOrdinalizer()); |
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.
Is this not going to break exiting pt-BR users?
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.
No. pt-PT and pt-BR is the same language, similar to en-US and en-GB. They have some particular differences in some cases, which isn't this case .
When the sub-set of the language is irrelevant we usually use just "pt" or just "en".
To be consistent I've changed the BrazilianPortugueseOrdinalizer to PortugueseOrdinalizer because the language is Portuguese, there aren't such thing as Brazilian-Portuguese language.
Is this forgotten? |
@TiagoBrito no, it is not forgotten. It LGTM, @mexx @MehdiK. The one thing, we probably need to do is squash the commits and rebase on top of master. Also, not directly related, but just wondering. What if we have "Resources.pt.resx" which contains all Portuguese resources, and "Resources.pt-BR.resx" which contains only Brasil specific wordings. Do you think it will work? |
@hazzik Yes, that is the best practice, and it will work. but as I told before, right now I didn't find any particular word to justify the existence of a "Resources.pt-BR.resx", but we can have it (empty or duplicated from "Resources.pt.resx") and if one day that case shows up, we can add it there. |
This is now merged. Thanks for the contribution. |
Thanks for the contribution. This is now released to NuGet as v1.37.0. |
Change BrazilianPortugueseOrdinalizer to PortugueseOrdinalizer there is no difference between Brazilian-PortugueseOrdinalizer and Portuguese.