I don't see most of the stuff marked "done" in the patch. Should I wait for another one?
On Tue, Sep 28, 2010 at 11:07 AM, <[email protected]> wrote: > > > > > 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) > On 2010/09/28 17:20:16, rjrjr wrote: > >> should not be public. >> > > Done. > > > 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); > On 2010/09/28 17:20:16, rjrjr wrote: > >> Do we have tests of the primitive param types? >> > > We do not have full coverage in RequestFactoryTest or > JsonRequestProcessorTest > > > 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) > On 2010/09/28 17:20:16, rjrjr wrote: > >> name > methodName >> > > Done. > > > 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, > On 2010/09/28 17:20:16, rjrjr wrote: > >> ditto >> > > Done. > > > 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()) > On 2010/09/28 17:20:16, rjrjr wrote: > >> Why not use cType here? >> > > Done. > > > 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()), > On 2010/09/28 17:20:16, rjrjr wrote: > >> The set and list checks could be factored into another method rather >> > than being > >> copy pasted like this, matchesErasedType(setType, cType) >> > > Done. > > > 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) > On 2010/09/28 17:20:16, rjrjr wrote: > >> 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. > > Done. > > > http://gwt-code-reviews.appspot.com/929801/diff/1/2#newcode339 > > user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:339: > } catch (DiagnosticException e) { > On 2010/09/28 17:20:16, rjrjr wrote: > >> Why not have a single catch around generateOnce? If you're worried >> > about > >> signature pollution make it a runtime execption >> > > I was worried about two things, 1) sig pollution, but 2) the possibility > later that we may want to report an error and continue with other proxy > types, rather then blowing up on the first error. > > > 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) { > They pretty much do the same thing, just with different APIs. The former > finds all methods of a given name that are GWT translatable > (client/shared). The latter finds all methods of a given name that we > have compiled bytecode on the classpath for (e.g. server classes), the > APIs are disjoint since java.lang.Reflect and GWT typeoracle are > disjoint. > > > > On 2010/09/28 17:20:16, rjrjr wrote: > >> 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#newcode775 > > user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:775: > continue; > On 2010/09/28 17:20:16, rjrjr wrote: > >> Why is this needed? Please document. If it's speculative, please >> > remove. > > It's not speculative, it skips over the stableId() method in > EntityProxy. > > > 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); > On 2010/09/28 17:20:16, rjrjr wrote: > >> Redundant check for this in maybeDecode, right? >> > > For maybeDecode, I need the String name of the domain class type as a > return value, because I'm comparing for equality between JClassType and > Class<?>. But for this method, I need the actual class. The solution is > to extract the ProxyFor lookup out of maybeDecode into a smaller method. > > > http://gwt-code-reviews.appspot.com/929801/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
