Community
Participate
Working Groups
From code inspection, it was found that most of nearly all of the error messages in the referenced package are hard-coded.
Please provide a sizing and include in your list of I6 defects. Please generate (see me for more information) message IDs for all error messages.
Steps required for this defect: 1) Determine the strings that need to be externalized. The JDT provides a sub-menu on the source context menu called "Externalize Strings" for searching for strings to be externalized to properties files: http://help.eclipse.org/help33/index.jsp?topic=/org.eclipse.jdt.doc.user/reference/ref-menu-source.htm 2) If the source folder does not have a resource bundle class/properties files, create them in an internal package named <source folder package prefix>.internal.resources (see http://www.eclipse.org/tptp/home/documents/process/development/api_contract.html), using the following files as templates: message.properties: ############################################################################### # Copyright (c) 2008 IBM Corporation and others. # All rights reserved. This program and the accompanying materials # are made available under the terms of the Eclipse Public License v1.0 # which accompanies this distribution, and is available at # http://www.eclipse.org/legal/epl-v10.html # $Id$ # # Contributors: # IBM Corporation - initial API and implementation ############################################################################### # # messages.properties # # NOTE: When using substitution parameters, all single quote characters (e.g. ') must be escaped with a preceding single quote character (e.g. ''text in single quotes''). # # NLS_MESSAGEFORMAT_VAR # NLS_ENCODING=UTF-8 # Messages for class <class name>.java: HELLO_WORLD = Hello World! <meaningful prefix>ResourceBundle.java: ********************************************************************** * Copyright (c) 2008 IBM Corporation and others. * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html * $Id$ * * Contributors: * IBM - Initial API and implementation **********************************************************************/ package <source folder package prefix>.internal.resources; import org.eclipse.osgi.util.NLS; /** * <meaningful prefix> resource bundle. * <p> * * * @author Tony Wang * @version March 10, 2008 * @since March 10, 2008 */ public final class <meaningful prefix>ResourceBundle extends NLS { private static final String BUNDLE_NAME = "<source folder package prefix>.internal.resources.messages";//$NON-NLS-1$ private ManualUIResourceBundle() { // Do not instantiate } public static String HELLO_WORLD; static { NLS.initializeMessages(BUNDLE_NAME, ManualUIResourceBundle.class); } } 3) The JDT provides a sub-menu on the source context menu called "Find Broken Externalized Strings" for searching for broken externalized strings in a selected property file, package, project or set of projects: http://help.eclipse.org/help33/index.jsp?topic=/org.eclipse.jdt.doc.user/reference/ref-menu-source.htm Since translation is of property string or resource bundles is costly, we should ensure unreferenced/unused translated property strings and their resource bundle keys are removed.
(In reply to comment #2) 4) Strings that should not be translated (e.g. constants, IDs, etc.) do not get externalized. Instead, //$NON-NLS-1$ is added at the end of the line with the string. Lines with two strings have //$NON-NLS-1$ //$NON-NLS-2$ at the end, and so on.
Created attachment 92586 [details] Patch version 1
Hi DuWayne, could review this patch? Thanks! And please also look at the broken strings problem in src-automation-client/org.eclipse.hyades.automation.client.adapters.shell
Filed a separate defect against ASF for the broken strings found in src-automation-client/org.eclipse.hyades.automation.client.adapters.shell. see https://bugs.eclipse.org/bugs/show_bug.cgi?id=222780. Should be a quick easy fix.
Finished review. Great job! Thanks. That was a very tedious job to do. I made minor changes and delivered. Most of my changes were to update the copyright date on various files. Took me about 3 hours just to review this large number of affected files. Tony, please add your hours worked to the total. I will attach my updated patch. Marking as fixed.
Created attachment 93060 [details] Modified patch Adding modified patch after review completed. This patch represents the delivered changes.
Marking as fixed.
add work hours
I think the mistake was made of externalizing all the FileSystemServices.println() statements. These statements are debug only. For instance, *FileCommand.java (in platform\org.eclipse.hyades.execution\src-core\org\eclipse\hyades\internal\execution\core\file), TCPDataServer, MultiplexedDataServer, FileServerExtended, etc. Any chance of getting them reverted back? I hate to have to go through three file steps to see what the error statement in the code actually is. ;)
Please address Jonathan's concerns
(In reply to comment #11) > I think the mistake was made of externalizing all the > FileSystemServices.println() statements. These statements are debug only. > > For instance, *FileCommand.java (in > platform\org.eclipse.hyades.execution\src-core\org\eclipse\hyades\internal\execution\core\file), > TCPDataServer, MultiplexedDataServer, FileServerExtended, etc. Any chance of > getting them reverted back? I hate to have to go through three file steps to > see what the error statement in the code actually is. ;) > What do you mean in your last sentence (e.g. three file steps)? If DEBUG is turned on, these statements should now be translated without any extra steps. Standard out and error statements should be logging to a local log file, using either Eclipse Logging or Java Logging. In this case, the org.eclipse.hyades.internal.execution.core.file.FileSystemServices.java class should be logging to a Java Logging log file (preferably as CBEs) so the developer/user can configure the logger and logging level at runtime. I have no problem reverting these string (especially if it reduces translation costs), but I want to make sure it is the right thing to be doing in this case.
Hi Paul, thanks for the quick response! (Sorry, my reply became longer than I expected it to be :) ) My concern over the three files steps occurs in two cases: - When encountering an error message in a log file, in order to determine it's origin I must: search the code for the string, to determine the properties file it is in; search the code for the constant listed in the properties file, to determine the Java file the constant is stored under; then search for the java file that references this constant. - Secondly, the error messages serve as defacto comments in the code. By seeing a logging statement like "TCPDataProcessor.run() - Monitoring is stopped, keep this thread in sleep state !", I know what the particular branch is handling. However, seeing "LocalPublicResourceBundle.TCPDataServer_DATA_PROCESSOR_RUN_" (what the first string has been replaced with), I have to go through the same three steps as above to determine what error or message is being logged. First search for the constant, then find the Java file this constant is referenced in, then find the properties file that contains the value of that constant. >> "Standard out and error statements should be logging to a local log file" Agreed. What the Hyades code is doing is not ideal, and the solution you suggest is certainly superior, e.g. logging to a log file and using a configurable log level. However as you can imagine it's unlikely we'll get multilevel debugging in the code. I think the important thing is that these statements are only printed when the debug constant is set in the code, something only a developer is able to do (or a user, instructed by a developer, with the output going to a developer). In this case, the console is acting as log for the file. I think generally debug statements aren't translated because the audience for the code is usually in the single or double digits (and the audience is already using the English only comments of the source). My comments are concerning only those bits of code that are developer only because they are conditional on code that is only set by the developer (directly or indirectly). Specifically the "if(CommunicationDebug.INSTANCE.debug) {}" statements in TCPDataServer, for instance, or the "FileSystemServices.println()" statements which use "FileSystemServices.Options.SHOW_DEBUG.isEnabled()". Those two examples take their values from undocumented environment variables passed into the JVM on program execution with the -D switch. On the other hand, the code which is user facing should most definitely be translated. This allows product consumers the ability to troubleshoot on their own. Let me know what you think. If you like I can generate a patch for the files I'm concerned about; using Beyond Compare and the Eclipse synchronize feature I can generate a patch fairly quickly. ;)
(In reply to comment #14) Hi Jonathan, I have added some comments below: > Hi Paul, thanks for the quick response! (Sorry, my reply became longer than I > expected it to be :) ) > > My concern over the three files steps occurs in two cases: > - When encountering an error message in a log file, in order to determine it's > origin I must: search the code for the string, to determine the properties file > it is in; search the code for the constant listed in the properties file, to > determine the Java file the constant is stored under; then search for the java > file that references this constant. This is simply a product of Eclipse 3.1+ message bundles and cannot be avoided. > - Secondly, the error messages serve as defacto comments in the code. By seeing > a logging statement like "TCPDataProcessor.run() - Monitoring is stopped, keep > this thread in sleep state !", I know what the particular branch is handling. > However, seeing "LocalPublicResourceBundle.TCPDataServer_DATA_PROCESSOR_RUN_" > (what the first string has been replaced with), I have to go through the same > three steps as above to determine what error or message is being logged. First > search for the constant, then find the Java file this constant is referenced > in, then find the properties file that contains the value of that constant. Logging messages should never replace well commented code:) It is a good programming practice to comment non-intuitive code and log user/system events and/or state changes useful for postmortem problem determination. Finally, errors should be logged and reported to user (e.g. error dialog). > >> "Standard out and error statements should be logging to a local log file" > Agreed. What the Hyades code is doing is not ideal, and the solution you > suggest is certainly superior, e.g. logging to a log file and using a > configurable log level. However as you can imagine it's unlikely we'll get > multilevel debugging in the code. I think the important thing is that these > statements are only printed when the debug constant is set in the code, > something only a developer is able to do (or a user, instructed by a developer, > with the output going to a developer). In this case, the console is acting as > log for the file. I think generally debug statements aren't translated because > the audience for the code is usually in the single or double digits (and the > audience is already using the English only comments of the source). in Java, debug statements should be logged to a configurable logger (e.g. logging at the DEBUG logging level) and a log file, standard out/error, or possibly a Logging Agent. Debug statements enabled by a run-time constant or environment variable is a C/++ programming practice used to circumvent the lack of good logging technologies, something that we do not have to worry about on the Java side. > My comments are concerning only those bits of code that are developer only > because they are conditional on code that is only set by the developer > (directly or indirectly). Specifically the > "if(CommunicationDebug.INSTANCE.debug) {}" statements in TCPDataServer, for > instance, or the "FileSystemServices.println()" statements which use > "FileSystemServices.Options.SHOW_DEBUG.isEnabled()". Those two examples take > their values from undocumented environment variables passed into the JVM on > program execution with the -D switch. Now that I have given my spiel on good programming practices, lets break them:) This situation is somewhat unique since we are deploying classes to a target run-time (a questionable design but that is another discussion), we want to minimize the number of deployed files, and these messages are not surfaced to the user in the UI or a log file, I would suggest we revert the changes ONLY for string in conditional output statements (e.g. if(DEBUG){System.out...). > On the other hand, the code which is user facing should most definitely be > translated. This allows product consumers the ability to troubleshoot on their > own. Agreed. > Let me know what you think. If you like I can generate a patch for the files > I'm concerned about; using Beyond Compare and the Eclipse synchronize feature I > can generate a patch fairly quickly. ;) We very much appreciate your offer and would love to take you up on it:) Please attach the patch to https://bugs.eclipse.org/bugs/show_bug.cgi?id=225672 so I can review. Thanks Jonathan for handling this regression.
Marking fixed again since the regression will be resolved under https://bugs.eclipse.org/bugs/show_bug.cgi?id=225672.
Verified in 4.5i8 and closing.
