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

Accept i18n errors #684

Merged
merged 9 commits into from
Oct 25, 2017
Merged

Accept i18n errors #684

merged 9 commits into from
Oct 25, 2017

Conversation

lauraluiz
Copy link
Contributor

@lauraluiz lauraluiz commented Oct 24, 2017

All error messages can be localized now.
The ones defined in Sunrise are located in the bundle "messages" (as shows the messages.yaml files). The ones pre-defined by Play (i.e. https://github.com/playframework/playframework/blob/2.5.18/framework/src/play/src/main/resources/messages.default) are located in the default bundle ("main" if not changed). They can also be localized using the default Play way of creating messages files for each language.

@zonorti zonorti temporarily deployed to ct-sunrise-stage-pr-684 October 24, 2017 10:23 Inactive
@lauraluiz lauraluiz temporarily deployed to ct-sunrise-stage-pr-684 October 24, 2017 12:41 Inactive
@lauraluiz lauraluiz requested review from katmatt and acbeni October 24, 2017 12:44
@lauraluiz lauraluiz temporarily deployed to ct-sunrise-stage-pr-684 October 24, 2017 13:04 Inactive
Copy link
Contributor

@katmatt katmatt left a comment

Choose a reason for hiding this comment

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

Looks already very good, but I found some smallish issues.

@@ -18,16 +21,22 @@
* @param message error message
* @return the error message localized and formatted
*/
String format(final List<Locale> locales, final String message);
String format(final List<Locale> locales, final String message, final Map<String, Object> hashArgs);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer the parameter name args here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named them hashArgs here and in I18nResolver because they are hash arguments. What don't you like in particular from the name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the type of hashArgs is just Map, I was wondering how useful it is to mention hash in the parameter name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -35,7 +32,7 @@ public void extractErrors() throws Exception {

private void checkError(ErrorBean error, String field, String key, String message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't check it in the IDE, but to me it looks the key parameter is unused?

import static org.mockito.Mockito.when;

@RunWith(MockitoJUnitRunner.class)
public class ErrorFormatterImplTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the new tests 👍

@lauraluiz lauraluiz temporarily deployed to ct-sunrise-stage-pr-684 October 24, 2017 14:07 Inactive
Copy link
Contributor Author

@lauraluiz lauraluiz left a comment

Choose a reason for hiding this comment

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

@katmatt changes done, please review :)

@@ -18,16 +21,22 @@
* @param message error message
* @return the error message localized and formatted
*/
String format(final List<Locale> locales, final String message);
String format(final List<Locale> locales, final String message, final Map<String, Object> hashArgs);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@acbeni
Copy link
Contributor

acbeni commented Oct 25, 2017

@lauraluiz i like what has been done in general, there is major quality amelioration that can be done and that i think is worth the effort, which is to work with generated classes like the android R.java which would allow us to get hold of all the messages in type safe way.

WDYT?

@lauraluiz
Copy link
Contributor Author

Having type-safe messages sounds really interesting @acbeni ! I hadn't thought of that solution. Could you please create an issue for that? I would address that after v1.0.0 though.

@lauraluiz
Copy link
Contributor Author

And actually all theme resources might benefit from it.

@acbeni
Copy link
Contributor

acbeni commented Oct 25, 2017

@lauraluiz i created issue to follow up #685

Copy link
Contributor

@katmatt katmatt 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 👍

@lauraluiz lauraluiz merged commit 1e2aba3 into pre-1.0.0 Oct 25, 2017
@lauraluiz lauraluiz deleted the i18n-errors branch October 25, 2017 15:39
@lauraluiz lauraluiz removed the review label Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants