You should be able to add JRE tests for these diagnostics, especially if
you factor them into a separate helper class. See EditorModelTest for an
example.
On 2010/09/28 17:20:16, rjrjr wrote:
Please fix DTRF and re-enable the commented out block before
submitting.
http://gwt-code-reviews.appspot.com/929801/diff/1/2
File
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java
(right):
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode157
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:157:
public void diagnosticIf(boolean cond, String fmt, Object... args)
should not be public.
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode192
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:192:
allowedRequestParamTypes.add(JPrimitiveType.CHAR);
Do we have tests of the primitive param types?
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode266
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:266:
private void diagnosticIfBannedType(JType origType, String entityName,
String
name)
name > methodName
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode275
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:275:
private void diagnosticIfBannedType(Set<JType> allowed, JType
origType,
ditto
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode285
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:285:
diagnosticIf(setType.isAssignableFrom(origType.isClassOrInterface())
Why not use cType here?
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode288
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:288:
&& !listType.getErasedType().equals(cType.getErasedType()),
The set and list checks could be factored into another method rather
than being
copy pasted like this, matchesErasedType(setType, cType)
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode292
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:292:
&& !entityProxyType.isAssignableFrom(origType.isClassOrInterface())
use cType
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode303
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:303:
if (setType.isAssignableFrom(typeArg)
Looks like you meant to say listType in one of these. Again, please
refactor and
reuse rather than copying and pasting, even for short things like
this.
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode339
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:339:
} catch (DiagnosticException e) {
Why not have a single catch around generateOnce? If you're worried
about
signature pollution make it a runtime execption
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode487
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:487:
private List<JMethod> findMethods(JClassType domainType, String name)
{
More descriptive names on the two findMethods, please, since they seem
to find
different things.
Or make a single method, and make the method collection its input
rather than
baking in the particular call?
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode645
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:645:
try {
Move to call to generateOnce
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode775
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:775:
continue;
Why is this needed? Please document. If it's speculative, please
remove.
http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode1329
user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:1329:
"Proxy %s is missing @ProxyFor annotation", entityName);
Redundant check for this in maybeDecode, right?
http://gwt-code-reviews.appspot.com/929801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
