[gwt-contrib] Re: Fix Eclipse / Checkstyle / Javadoc warnings (issue954801)
Mostly LGTM with some small suggestions. However, I think there is one big thing that needs further discussion -- I think instead of trying to change the code, I suggest we change the Eclipse warnings to not warn about unused parameters in implemented/overridden methods. http://gwt-code-reviews.appspot.com/954801/diff/1/6 File dev/core/src/com/google/gwt/dev/jjs/impl/OptimizerStats.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/6#newcode91 dev/core/src/com/google/gwt/dev/jjs/impl/OptimizerStats.java:91: * Return a human readable string representing the values of all statistics. human-readable http://gwt-code-reviews.appspot.com/954801/diff/1/16 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/16#newcode104 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java:104: void onCancel(ClickEvent event) { Put the @SuppressWarnings on the parameter rather than the method, here and below. However, I am not sure if we want to do this -- it seems pretty common that parameters on callbacks will be unused, and so I think it is better to have Ignore in overriding and implementing methods checked in the Eclipse warnings configuration. http://gwt-code-reviews.appspot.com/954801/diff/1/19 File samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java (left): http://gwt-code-reviews.appspot.com/954801/diff/1/19#oldcode46 samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java:46: import com.google.gwt.requestfactory.shared.Request; Why wasn't this caught by ant checkstyle? http://gwt-code-reviews.appspot.com/954801/diff/1/21 File samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/21#newcode61 samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java:61: private final Listener listener; Why do we need the field at all if it is unused? http://gwt-code-reviews.appspot.com/954801/diff/1/32 File user/src/com/google/gwt/app/place/Prefix.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/32#newcode27 user/src/com/google/gwt/app/place/Prefix.java:27: * {code com.google.gwt.app.rebind.PlaceHistoryMapperGenerator} looks Why not link? http://gwt-code-reviews.appspot.com/954801/diff/1/36 File user/src/com/google/gwt/editor/client/AutoBean.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/36#newcode54 user/src/com/google/gwt/editor/client/AutoBean.java:54: * Returns codetrue/code if {...@link #setFrozen} has been called. Shouldn't it talk about being last called with true? It looks like the freeze=setFrozen change was to allow thawing a frozen object. http://gwt-code-reviews.appspot.com/954801/diff/1/37 File user/src/com/google/gwt/editor/client/AutoBeanVisitor.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/37#newcode52 user/src/com/google/gwt/editor/client/AutoBeanVisitor.java:52: * TODO: document. Should probably ask the person who wrote these methods to provide the description. http://gwt-code-reviews.appspot.com/954801/diff/1/44 File user/src/com/google/gwt/event/shared/EventBus.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/44#newcode27 user/src/com/google/gwt/event/shared/EventBus.java:27: * p See {...@code com.google.gwt.event.shared.testing.CountingEventBus}. Why this change? And if you do want to move it into the main doc, why not @link? http://gwt-code-reviews.appspot.com/954801/diff/1/50 File user/src/com/google/gwt/logging/client/HtmlLogFormatter.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/50#newcode62 user/src/com/google/gwt/logging/client/HtmlLogFormatter.java:62: * @param event Why use this method here and @SuppressWarnings elsewhere? Again, I am not sure we want to make these changes rather than just change Eclipse's warnings settings (I believe what you get by following eclipse/README.txt will not warn on these). http://gwt-code-reviews.appspot.com/954801/diff/1/65 File user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/65#newcode31 user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java:31: * 064;Template(span class=\{3}\{0}: a href=\{1}\{2}/a/span) Isn't this supposed to be #064;? http://gwt-code-reviews.appspot.com/954801/diff/1/68 File user/src/com/google/gwt/uibinder/client/UiChild.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/68#newcode43 user/src/com/google/gwt/uibinder/client/UiChild.java:43: * 064;UiChild MyWidget#addCustomChild(Widget w) /code and And here. http://gwt-code-reviews.appspot.com/954801/diff/1/72 File user/src/com/google/gwt/user/client/ResponseTextHandler.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/72#newcode18
[gwt-contrib] Re: Fix Eclipse / Checkstyle / Javadoc warnings (issue954801)
http://gwt-code-reviews.appspot.com/954801/diff/1/6 File dev/core/src/com/google/gwt/dev/jjs/impl/OptimizerStats.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/6#newcode91 dev/core/src/com/google/gwt/dev/jjs/impl/OptimizerStats.java:91: * Return a human readable string representing the values of all statistics. OK On 2010/10/05 17:34:21, jat wrote: human-readable http://gwt-code-reviews.appspot.com/954801/diff/1/16 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/16#newcode104 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java:104: void onCancel(ClickEvent event) { It's already checked. On 2010/10/05 17:34:21, jat wrote: Put the @SuppressWarnings on the parameter rather than the method, here and below. However, I am not sure if we want to do this -- it seems pretty common that parameters on callbacks will be unused, and so I think it is better to have Ignore in overriding and implementing methods checked in the Eclipse warnings configuration. http://gwt-code-reviews.appspot.com/954801/diff/1/19 File samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java (left): http://gwt-code-reviews.appspot.com/954801/diff/1/19#oldcode46 samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java:46: import com.google.gwt.requestfactory.shared.Request; No idea :-( On 2010/10/05 17:34:21, jat wrote: Why wasn't this caught by ant checkstyle? http://gwt-code-reviews.appspot.com/954801/diff/1/21 File samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/21#newcode61 samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java:61: private final Listener listener; I think it might be used by generated code but I'm not sure about that. On 2010/10/05 17:34:21, jat wrote: Why do we need the field at all if it is unused? http://gwt-code-reviews.appspot.com/954801/diff/1/32 File user/src/com/google/gwt/app/place/Prefix.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/32#newcode27 user/src/com/google/gwt/app/place/Prefix.java:27: * {code com.google.gwt.app.rebind.PlaceHistoryMapperGenerator} looks Will link and include destination class On 2010/10/05 17:34:21, jat wrote: Why not link? http://gwt-code-reviews.appspot.com/954801/diff/1/36 File user/src/com/google/gwt/editor/client/AutoBean.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/36#newcode54 user/src/com/google/gwt/editor/client/AutoBean.java:54: * Returns codetrue/code if {...@link #setFrozen} has been called. Fixed On 2010/10/05 17:34:21, jat wrote: Shouldn't it talk about being last called with true? It looks like the freeze=setFrozen change was to allow thawing a frozen object. http://gwt-code-reviews.appspot.com/954801/diff/1/37 File user/src/com/google/gwt/editor/client/AutoBeanVisitor.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/37#newcode52 user/src/com/google/gwt/editor/client/AutoBeanVisitor.java:52: * TODO: document. Will do. On 2010/10/05 17:34:21, jat wrote: Should probably ask the person who wrote these methods to provide the description. http://gwt-code-reviews.appspot.com/954801/diff/1/44 File user/src/com/google/gwt/event/shared/EventBus.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/44#newcode27 user/src/com/google/gwt/event/shared/EventBus.java:27: * p See {...@code com.google.gwt.event.shared.testing.CountingEventBus}. I'll @link it and add the target to the javadoc build. On 2010/10/05 17:34:21, jat wrote: Why this change? And if you do want to move it into the main doc, why not @link? http://gwt-code-reviews.appspot.com/954801/diff/1/50 File user/src/com/google/gwt/logging/client/HtmlLogFormatter.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/50#newcode62 user/src/com/google/gwt/logging/client/HtmlLogFormatter.java:62: * @param event My scheme is more or less as follows: If the unused param occurs in 'frameworky' code, where we expect there to be subclasses that make use of the param, javadoc it If the method occurs in code that is unlikely to be further subclassed (but isn't one of the situations where checking the box in Eclipse will remove the warning, such as can happen with UiBinder), use @SuppressWarnings On 2010/10/05 17:34:21, jat wrote: Why use this method here and @SuppressWarnings elsewhere? Again, I am not sure we want to make these changes rather than just change Eclipse's warnings settings (I believe what you get by following eclipse/README.txt will not warn on these). http://gwt-code-reviews.appspot.com/954801/diff/1/65 File user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java (right):
[gwt-contrib] Re: Fix Eclipse / Checkstyle / Javadoc warnings (issue954801)
LGTM http://gwt-code-reviews.appspot.com/954801/diff/1/19 File samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java (left): http://gwt-code-reviews.appspot.com/954801/diff/1/19#oldcode46 samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java:46: import com.google.gwt.requestfactory.shared.Request; Doesn't have to be done for this change, but we should figure out why as unused imports should trigger a checkstyle failure. http://gwt-code-reviews.appspot.com/954801/diff/1/21 File samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/21#newcode61 samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java:61: private final Listener listener; On 2010/10/05 20:11:46, rice wrote: I think it might be used by generated code but I'm not sure about that. If it is private, how could it? Or are you suggesting generated JSNI code could be using it? http://gwt-code-reviews.appspot.com/954801/diff/1/32 File user/src/com/google/gwt/app/place/Prefix.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/32#newcode27 user/src/com/google/gwt/app/place/Prefix.java:27: * {code com.google.gwt.app.rebind.PlaceHistoryMapperGenerator} looks On 2010/10/05 20:11:46, rice wrote: Will link and include destination class Sorry, didn't notice it was a rebind class that we plan to exclude from Javadoc. It is fine as-is. http://gwt-code-reviews.appspot.com/954801/diff/1/50 File user/src/com/google/gwt/logging/client/HtmlLogFormatter.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/50#newcode62 user/src/com/google/gwt/logging/client/HtmlLogFormatter.java:62: * @param event On 2010/10/05 20:11:46, rice wrote: My scheme is more or less as follows: If the unused param occurs in 'frameworky' code, where we expect there to be subclasses that make use of the param, javadoc it If the method occurs in code that is unlikely to be further subclassed (but isn't one of the situations where checking the box in Eclipse will remove the warning, such as can happen with UiBinder), use @SuppressWarnings Ok. http://gwt-code-reviews.appspot.com/954801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors