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

some messages are still hardcoded in english #3110

Open
rnveach opened this Issue Apr 16, 2016 · 21 comments

Comments

Projects
None yet
4 participants
@rnveach
Member

rnveach commented Apr 16, 2016

CS still has some output events that are hardcoded in the code and are only in English. Some already have translations in message files, but we don't use them.
We translate check messages to their specific language so non-english users can understand the issue, we should do the same with every output of CS (minus XML attributes and such) so there is no confusion.

Examples:

errorWriter.println("Error auditing " + event.getFileName());
throwable.printStackTrace(errorWriter);
}
}
@Override
public void auditStarted(AuditEvent event) {
infoWriter.println("Starting audit...");
infoWriter.flush();
}
@Override
public void auditFinished(AuditEvent event) {
infoWriter.println("Audit done.");

System.out.println(String.format("Checkstyle ends with %d errors.", errorCounter));

@rnveach rnveach added the easy label Mar 2, 2017

@MaggieMa21

This comment has been minimized.

Show comment
Hide comment
@MaggieMa21

MaggieMa21 Mar 15, 2017

I am on this.

MaggieMa21 commented Mar 15, 2017

I am on this.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 24, 2017

Contributor

@romani @rnveach Could you provide any other examples for translated messages?

Contributor

subkrish commented Jun 24, 2017

@romani @rnveach Could you provide any other examples for translated messages?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 24, 2017

Member

Why this example is not enough ? Please share your problem

Member

romani commented Jun 24, 2017

Why this example is not enough ? Please share your problem

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 24, 2017

Contributor

@romani I am having a trouble in trying to understand what class to import to use localized messages. I was trying to follow how localized messages have been used elsewhere. But due to different implementations, I wasn't sure how to implement it.

I just wanted a little more clarity.

Contributor

subkrish commented Jun 24, 2017

@romani I am having a trouble in trying to understand what class to import to use localized messages. I was trying to follow how localized messages have been used elsewhere. But due to different implementations, I wasn't sure how to implement it.

I just wanted a little more clarity.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 24, 2017

Member

it is better to try another issue for you.

Member

romani commented Jun 24, 2017

it is better to try another issue for you.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 24, 2017

Contributor

@romani Okay but I give trying to solve the issue my best shot and if I fall short I'll work on a different issue.

Contributor

subkrish commented Jun 24, 2017

@romani Okay but I give trying to solve the issue my best shot and if I fall short I'll work on a different issue.

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Jun 26, 2017

romani added a commit that referenced this issue Jun 26, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 26, 2017

Member

@subkrish , please proceed with hardcoded lines at:

  • please investigate why DefaultLoggerTest is NOT failed in localized UTs, as "Error auditing" is hard coded in UT in English.
  • Main
  • Checker
  • please recheck that non of Exception throw message is left with hardcoded content (152 cases as minimum):
~/java/git-others/checkstyle/checkstyle/src/main/java/com/puppycrawl/tools [master|✔ ] 
$ grep -R "throw new" .* | grep "\""
./checkstyle/PropertyCacheFile.java:            throw new IllegalArgumentException("config can not be null");
./checkstyle/PropertyCacheFile.java:            throw new IllegalArgumentException("fileName can not be null");
./checkstyle/PropertyCacheFile.java:            throw new IllegalStateException("Unable to calculate hashcode.", ex);
....
Member

romani commented Jun 26, 2017

@subkrish , please proceed with hardcoded lines at:

  • please investigate why DefaultLoggerTest is NOT failed in localized UTs, as "Error auditing" is hard coded in UT in English.
  • Main
  • Checker
  • please recheck that non of Exception throw message is left with hardcoded content (152 cases as minimum):
~/java/git-others/checkstyle/checkstyle/src/main/java/com/puppycrawl/tools [master|✔ ] 
$ grep -R "throw new" .* | grep "\""
./checkstyle/PropertyCacheFile.java:            throw new IllegalArgumentException("config can not be null");
./checkstyle/PropertyCacheFile.java:            throw new IllegalArgumentException("fileName can not be null");
./checkstyle/PropertyCacheFile.java:            throw new IllegalStateException("Unable to calculate hashcode.", ex);
....
@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 27, 2017

Contributor

@romani @rnveach Looks like the message for this

System.out.println(String.format("Checkstyle ends with %d errors.", errorCounter));
is not present in the 'messages.properties' file and the subsequent other languages files. Do I add them for the languages present and then remove the hardcoded lines? I might have to use a translator for the other languages messages.

Contributor

subkrish commented Jun 27, 2017

@romani @rnveach Looks like the message for this

System.out.println(String.format("Checkstyle ends with %d errors.", errorCounter));
is not present in the 'messages.properties' file and the subsequent other languages files. Do I add them for the languages present and then remove the hardcoded lines? I might have to use a translator for the other languages messages.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 27, 2017

Member

Do I add them for the languages present and then remove the hardcoded lines?

Yes.

I might have to use a translator for the other languages messages.

That is what everyone does. No one in Checkstyle of us know all these languages.
If translation is horrible, someone will make a request to have it fixed.

Member

rnveach commented Jun 27, 2017

Do I add them for the languages present and then remove the hardcoded lines?

Yes.

I might have to use a translator for the other languages messages.

That is what everyone does. No one in Checkstyle of us know all these languages.
If translation is horrible, someone will make a request to have it fixed.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 27, 2017

Contributor

@romani @rnveach for the Main file changes should also be made to command line option that only print in english right?
For example:

// show version and exit if it is requested
            if (commandLine.hasOption(OPTION_V_NAME)) {
                System.out.println("Checkstyle version: "
                        + Main.class.getPackage().getImplementationVersion());
                exitStatus = 0;
            }

and https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L614-L636

Contributor

subkrish commented Jun 27, 2017

@romani @rnveach for the Main file changes should also be made to command line option that only print in english right?
For example:

// show version and exit if it is requested
            if (commandLine.hasOption(OPTION_V_NAME)) {
                System.out.println("Checkstyle version: "
                        + Main.class.getPackage().getImplementationVersion());
                exitStatus = 0;
            }

and https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L614-L636

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 28, 2017

Member

no changes to CLI is required, behavior should be the same as Check messages:

  • use default locale
  • if no translations for default locale, use default (english)
Member

romani commented Jun 28, 2017

no changes to CLI is required, behavior should be the same as Check messages:

  • use default locale
  • if no translations for default locale, use default (english)
@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 29, 2017

Contributor

@romani @rnveach

please investigate why DefaultLoggerTest is NOT failed in localized UTs, as "Error auditing" is hard coded in UT in English.

After some researching, I found out that maven runs the test in a separate forked process.

Please look at this:

Contributor

subkrish commented Jun 29, 2017

@romani @rnveach

please investigate why DefaultLoggerTest is NOT failed in localized UTs, as "Error auditing" is hard coded in UT in English.

After some researching, I found out that maven runs the test in a separate forked process.

Please look at this:

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 29, 2017

Member

maven runs the test in a separate forked process.

But this is not affecting our other tests, right? Why is it just affecting these?
I am pretty sure I have seen locale tests fail before.

Ex:
#3377
#4003
#3989
#3896
And so on....

Member

rnveach commented Jun 29, 2017

maven runs the test in a separate forked process.

But this is not affecting our other tests, right? Why is it just affecting these?
I am pretty sure I have seen locale tests fail before.

Ex:
#3377
#4003
#3989
#3896
And so on....

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 29, 2017

Contributor

@rnveach @romani

I tried running this command -> mvn clean integration-test failsafe:verify -DargLine='-Duser.language=de -Duser.country=DE' but DefaultLoggerTest didn't fail as it was supposed to because the message is hardcoded in the test:

assertTrue(output.contains("Error auditing myfile"));

But when the same option -> -Duser.language=de -Duser.country=DE is set in the POM file the test failed:

[INFO] Running com.puppycrawl.tools.checkstyle.DefaultLoggerTest
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.001 s <<< FAILURE! - in com.puppycrawl.tools.checkstyle.DefaultLoggerTest
[ERROR] testCtor(com.puppycrawl.tools.checkstyle.DefaultLoggerTest)  Time elapsed: 0 s  <<< FAILURE!
java.lang.AssertionError
	at com.puppycrawl.tools.checkstyle.DefaultLoggerTest.testCtor(DefaultLoggerTest.java:43)

I don't understand the difference between the two. I was assuming both give the same result, either UTs fails or passes.

EDIT: Looks like if the command is run as mvn clean integration-test failsafe:verify -DargLine='-Xms1024m -Xmx2048m -Duser.language=de -Duser.region=DE' then the UTs are failing as expected. I removed <argLine>-Xms1024m -Xmx2048m</argLine> from POM and then ran the command.

Contributor

subkrish commented Jun 29, 2017

@rnveach @romani

I tried running this command -> mvn clean integration-test failsafe:verify -DargLine='-Duser.language=de -Duser.country=DE' but DefaultLoggerTest didn't fail as it was supposed to because the message is hardcoded in the test:

assertTrue(output.contains("Error auditing myfile"));

But when the same option -> -Duser.language=de -Duser.country=DE is set in the POM file the test failed:

[INFO] Running com.puppycrawl.tools.checkstyle.DefaultLoggerTest
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.001 s <<< FAILURE! - in com.puppycrawl.tools.checkstyle.DefaultLoggerTest
[ERROR] testCtor(com.puppycrawl.tools.checkstyle.DefaultLoggerTest)  Time elapsed: 0 s  <<< FAILURE!
java.lang.AssertionError
	at com.puppycrawl.tools.checkstyle.DefaultLoggerTest.testCtor(DefaultLoggerTest.java:43)

I don't understand the difference between the two. I was assuming both give the same result, either UTs fails or passes.

EDIT: Looks like if the command is run as mvn clean integration-test failsafe:verify -DargLine='-Xms1024m -Xmx2048m -Duser.language=de -Duser.region=DE' then the UTs are failing as expected. I removed <argLine>-Xms1024m -Xmx2048m</argLine> from POM and then ran the command.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 30, 2017

Member

@subkrish , good catch.

it is caused by fix for #4316 , I send PR to address it #4558 .

Member

romani commented Jun 30, 2017

@subkrish , good catch.

it is caused by fix for #4316 , I send PR to address it #4558 .

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 30, 2017

Contributor

@romani This means that now DefaultLoggerTest will fail as the messages are still hardcoded. I would need to make a new PR to change those UTs as well.

EDIT: I will make those changes first and then I suppose PR #4558 can be merged because the localized tests will fail as mentioned above.

Contributor

subkrish commented Jun 30, 2017

@romani This means that now DefaultLoggerTest will fail as the messages are still hardcoded. I would need to make a new PR to change those UTs as well.

EDIT: I will make those changes first and then I suppose PR #4558 can be merged because the localized tests will fail as mentioned above.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 30, 2017

Member

my PR is failed, that is good, please provide PR with fix for localized message

Member

romani commented Jun 30, 2017

my PR is failed, that is good, please provide PR with fix for localized message

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jun 30, 2017

Contributor

@romani I have just created PR #4559 fixing those violations.

Contributor

subkrish commented Jun 30, 2017

@romani I have just created PR #4559 fixing those violations.

subkrish added a commit to subkrish/checkstyle that referenced this issue Jun 30, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jun 30, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jun 30, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jun 30, 2017

romani added a commit that referenced this issue Jun 30, 2017

romani added a commit that referenced this issue Jun 30, 2017

romani added a commit that referenced this issue Jun 30, 2017

romani added a commit that referenced this issue Jun 30, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 1, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 1, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 1, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 2, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 2, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 2, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 4, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 6, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 6, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 6, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 7, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 7, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 7, 2017

romani added a commit that referenced this issue Jul 8, 2017

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 8, 2017

Contributor

@romani
The files that are still left

  • Checker
  • Exception throw message that is left with hardcoded content (152 cases as minimum) (taken from #3110 (comment))

I will continue to make those changes under this issue.

Contributor

subkrish commented Jul 8, 2017

@romani
The files that are still left

  • Checker
  • Exception throw message that is left with hardcoded content (152 cases as minimum) (taken from #3110 (comment))

I will continue to make those changes under this issue.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 27, 2017

Member

final String message = "Should be only one tag.";
: This should probably also be translated.

Member

rnveach commented Jul 27, 2017

final String message = "Should be only one tag.";
: This should probably also be translated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment