> On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GlobalId.java, > > line 144 > > <https://reviews.apache.org/r/4268/diff/21/?file=109209#file109209line144> > > > > Small nit on style here. You're using the getters on the local object > > and accessing the fields directly on the other object.
HAHAHAHA! Interesting... fixed > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java, > > line 100 > > <https://reviews.apache.org/r/4268/diff/21/?file=109210#file109210line100> > > > > Small nit on style. The else if statements aren't needed because > > you're returning. These can be consecutive ifs. I'm gonna go ahead a leave it the way it is so the diff is not so cluttered. Seems easier to read the way it is. > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java, > > line 125 > > <https://reviews.apache.org/r/4268/diff/21/?file=109210#file109210line125> > > > > Small nit on style. These can be consecutive ifs because of the > > returns. Same comment as above > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java, > > line 146 > > <https://reviews.apache.org/r/4268/diff/21/?file=109210#file109210line146> > > > > Small nit on style. Don't need the else because of the return in the > > if body. Same comment as above > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/LocalId.java, > > line 22 > > <https://reviews.apache.org/r/4268/diff/21/?file=109211#file109211line22> > > > > Use IllegalArgumentException, please. Got it > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/LocalId.java, > > line 52 > > <https://reviews.apache.org/r/4268/diff/21/?file=109211#file109211line52> > > > > Same comments as for the domain name regex. Same comment as above > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/ObjectId.java, > > line 22 > > <https://reviews.apache.org/r/4268/diff/21/?file=109212#file109212line22> > > > > Use IllegalArgumentException instead, please. Got it > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java, > > line 54 > > <https://reviews.apache.org/r/4268/diff/21/?file=109210#file109210line54> > > > > throw IllegalArgumentException, please. Got it > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/DomainName.java, > > line 52 > > <https://reviews.apache.org/r/4268/diff/21/?file=109208#file109208line52> > > > > Should the "." in this regex be escaped with "\\"? So the regex would > > be "[a-zA-Z0-9_\\.-]*" You might be able to use \w to represent a word in > > this case. RegEx changed to \\w[a-zA-Z0-9_\\.-]* > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/DomainName.java, > > line 81 > > <https://reviews.apache.org/r/4268/diff/21/?file=109208#file109208line81> > > > > IllegalArgumentException, please. Got it > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GlobalId.java, > > line 44 > > <https://reviews.apache.org/r/4268/diff/21/?file=109209#file109209line44> > > > > throw IllegalArgumentException, please. Got it > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GlobalId.java, > > line 75 > > <https://reviews.apache.org/r/4268/diff/21/?file=109209#file109209line75> > > > > throw IllegalArgumentException, please. Got it > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/DomainName.java, > > line 42 > > <https://reviews.apache.org/r/4268/diff/21/?file=109208#file109208line42> > > > > This should be throwing IllegalArgumentException and not > > InvalidParameterException Got it > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/Group.java, > > line 133 > > <https://reviews.apache.org/r/4268/diff/21/?file=109207#file109207line133> > > > > This is no longer just valid GroupId object. It can be a valid GroupId > > representation. changed wording > On 2012-05-21 19:24:30, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/model/GroupImpl.java, > > line 60 > > <https://reviews.apache.org/r/4268/diff/21/?file=109206#file109206line60> > > > > I think this should be an IllegalArgumentException and not > > InvalidParameterException, which resides in java.security. Got it - Mike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4268/#review8017 ----------------------------------------------------------- On 2012-05-17 14:02:58, Mike May wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4268/ > ----------------------------------------------------------- > > (Updated 2012-05-17 14:02:58) > > > Review request for shindig, Henry Saputra, Ryan Baxter, and Stanton Sievers. > > > Summary > ------- > > A base implementation for OpenSocial Groups. Provides groups to the osapi > javascript namespace with an implementation link for get() in GroupService. > > > This addresses bug SHINDIG-1780. > https://issues.apache.org/jira/browse/SHINDIG-1780 > > > Diffs > ----- > > http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SocialHelloWorld.xml > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/content/sampledata/canonicaldb.json > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/main/java/org/apache/shindig/social/opensocial/jpa/spi/ActivityServiceDb.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/main/java/org/apache/shindig/social/opensocial/jpa/spi/AppDataServiceDb.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/main/java/org/apache/shindig/social/opensocial/jpa/spi/PersonServiceDb.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/test/java/org/apache/shindig/social/opensocial/jpa/spi/SpiDatabaseBootstrap.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/test/java/org/apache/shindig/social/opensocial/jpa/spi/SpiTestUtil.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/test/resources/sampledata/canonicaldb.json > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/config/SocialApiGuiceModule.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/model/GroupImpl.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/Group.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/DomainName.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GlobalId.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/LocalId.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/ObjectId.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/sample/SampleModule.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/sample/spi/JsonDbOpensocialService.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/dataservice/integration/RestfulJsonPeopleTest.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/dataservice/integration/RestfulXmlPeopleTest.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/DomainNameTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GlobalIdTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java > 1339181 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/LocalIdTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/ObjectIdTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/4268/diff > > > Testing > ------- > > Used the common container with a gadget to call osapi.groups.get() > > -- > > Created and updated unit tests > All tests pass > > > Thanks, > > Mike > >
