[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
