Other than the null comment...LGTM Applied patch and ran tests, all successful.
Ship it! http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java File java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java (right): http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java#newcode128 java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java:128: // return null we don't know the type +1 On 2012/06/11 13:48:55, DouglasLDavies wrote:
change comment about null
http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java File java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java (right): http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java#newcode43 java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java:43: GroupId unknown = GroupId.fromJson("@foo"); I would disagree, seems misleading to call it @custom. @foo demonstrates it can be anything and it not just another hardcoded type. Works either way but I think @foo is a better representation of the feature. On 2012/06/11 13:48:55, DouglasLDavies wrote:
Call the variable custom
http://codereview.appspot.com/6305079/
