Thanks very much for the review! Your point is taken on tests; I'm currently in the process of writing some rudimentary tests for JSON-RPC in RequestFactory.
As you mention, these tests will not be integration-level tests at first, as there is no RF server side in the testing infrastructure (as yet), but this will be a start. Expect another patch with tests shortly. http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java File user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java (right): http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode269 user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:269: assert (obj instanceof Collection); On 2011/12/20 10:51:18, tbroyer wrote:
How about casting 'obj' to a Collection at the call site instead? (and
thus
declare the argument here of type Collection)
Good idea, done. http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode274 user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:274: sb.append(nonCollectionEncode(it.next())); On 2011/12/20 10:51:18, tbroyer wrote:
Using encode() here would pave the way for supporting collections of
collections
(see issue 5974). You'd then turn the RuntimeException in nonCollectionEncode into a
simple
'assert'. I mean, in the case of JsonRpc, GWT doesn't provide the backend, so
there's
theoretically no reason to disallow collections of collections, except
for
parity with the "standard" RF protocol.
Agreed, but I'd rather put a TODO here and then fix 5974 on both sides - is that ok with you? http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode291 user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:291: if (obj.getClass().isEnum() && getAutoBeanFactory() instanceof EnumMap) { On 2011/12/20 10:51:18, tbroyer wrote:
Hmm, looks like there's the same issue as was fixed some time ago in
ValueCodex
re. enum values with their own "body", or when the enum has a
constructor with
arguments: in this case, their class is an anonymous subclass of the
enum class,
one for which isEnum() returns false. Using "obj instanceof Enum" instead of obj.getClass().isEnum() would
fix it.
See issue 6504.
Good catch. Fixed. http://gwt-code-reviews.appspot.com/1618806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
