The only blocker bug I see on your list is the one about the pom.xml tweak for Rajeev.
https://jira.springsource.org/secure/IssueNavigator.jspa?mode=hide&requestId=12040 The JRE tests are actually pretty easy to write, especially if you copy com.google.gwt.editor.rebind.model.EditorModelTest.EmptyMockJavaResource (with a pointer back to the original, so that we go back and consolidate these things). We need to stop committing code without coverage. On Tue, Sep 28, 2010 at 11:07 AM, <[email protected]> wrote: > > What is the time frame? Creating JRE tests looks to be a non-trivial > amount of work, if there's something else critical I need to get done > today (like fix addon-gwt 1-many), should I shelve this? > > > On 2010/09/28 17:30:43, rjrjr wrote: > >> 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
