|
|
Patch Set 1 #
Total comments: 6
Patch Set 2 : Updated gan sample to use shared cmdline sample + fixes #
Total comments: 5
Total messages: 11
|
namos
|
14 years, 5 months ago (2011年08月16日 17:25:56 UTC) #1 | |||||||||||||||||||||||||||||||
On 2011年08月16日 17:25:56, namos wrote: Bump.
Sending you some high-level comments to start the process. http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/pom.xml File gan-json-oauth-sample/pom.xml (right): http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/pom.xml#ne... gan-json-oauth-sample/pom.xml:11: <version>1.6.0-beta-SNAPSHOT</version> let's not depend on 1.6 in samples. Please instead depend on 1.5.0-beta. http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/src/com/go... File gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java (right): http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/src/com/go... gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:39: public class GanSample { How about putting a link somewhere to the code.google.com documentation for the GAN API? http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/src/com/go... File gan-json-oauth-sample/src/com/google/api/client/sample/gan/OAuth2.java (right): http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/src/com/go... gan-json-oauth-sample/src/com/google/api/client/sample/gan/OAuth2.java:38: public class OAuth2 { I'm adding a shared-sample-cmdline project that has OAuth 2 for command-line apps. Please wait for the following changeset to be submitted and then you can use it from your sample: http://codereview.appspot.com/4879041/ http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/src/com/go... File gan-json-oauth-sample/src/com/google/api/client/sample/gan/model/com/google/api/services/gan/Gan.java (right): http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/src/com/go... gan-json-oauth-sample/src/com/google/api/client/sample/gan/model/com/google/api/services/gan/Gan.java:49: public class Gan extends ApiClient { Please don't put these generated files into the sample. We have an internal process for pushing these generated jars into our Maven repository, so please please follow that.
Sorry for the delay on this, we've been working out some issues with getting the API released. http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/pom.xml File gan-json-oauth-sample/pom.xml (right): http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/pom.xml#ne... gan-json-oauth-sample/pom.xml:11: <version>1.6.0-beta-SNAPSHOT</version> On 2011年08月19日 14:05:25, yanivi wrote: > let's not depend on 1.6 in samples. Please instead depend on 1.5.0-beta. Done. http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/src/com/go... File gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java (right): http://codereview.appspot.com/4899045/diff/1/gan-json-oauth-sample/src/com/go... gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:39: public class GanSample { On 2011年08月19日 14:05:25, yanivi wrote: > How about putting a link somewhere to the http://code.google.com documentation for the > GAN API? Done.
http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml File gan-json-oauth-sample/pom.xml (right): http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml... gan-json-oauth-sample/pom.xml:10: <artifactId>gan-json-oauth-sample</artifactId> This directory and artifactId MUST be called gan-cmdline-sample to follow our new naming convention. http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml... gan-json-oauth-sample/pom.xml:102: <groupId>com.google.apis-samples</groupId> Missing dependency on google-api-services-gan jar, which again is still not in our mavenrepo. This is a requirement for all of our samples, because one cannot run the sample without the jar the sample depends on. http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... File gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java (right): http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:17: import com.google.api.services.gan.Gan; when running mvn compile I get this: /home/yanivi/hg/google-api-java-client/samples/default/gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:[17,34] package com.google.api.services.gan does not exist See my other comment. http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:41: * See for GAN API documentation: https://code.google.com/apis/gan/index.html prefer regular HTML syntax in JavaDoc, e.g. See <a href="https://code.google.com/apis/gan/index.html">GAN API documentation</a> http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:69: //Set up server normally there is a space after // so would normally look like: // Set up server
Is there a doc that describes everything I need to do to check in the maven jar? Like how to generate the md5/sha and xml? Also, do we need to include anything but what the library generator gives in the jar file? On 2011年09月09日 21:07:51, yanivi wrote: > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml > File gan-json-oauth-sample/pom.xml (right): > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml... > gan-json-oauth-sample/pom.xml:10: <artifactId>gan-json-oauth-sample</artifactId> > This directory and artifactId MUST be called gan-cmdline-sample to follow our > new naming convention. > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml... > gan-json-oauth-sample/pom.xml:102: <groupId>com.google.apis-samples</groupId> > Missing dependency on google-api-services-gan jar, which again is still not in > our mavenrepo. This is a requirement for all of our samples, because one cannot > run the sample without the jar the sample depends on. > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... > File gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java > (right): > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... > gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:17: > import com.google.api.services.gan.Gan; > when running mvn compile I get this: > > /home/yanivi/hg/google-api-java-client/samples/default/gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:[17,34] > package com.google.api.services.gan does not exist > > See my other comment. > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... > gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:41: * > See for GAN API documentation: https://code.google.com/apis/gan/index.html > prefer regular HTML syntax in JavaDoc, e.g. > > See <a href="https://code.google.com/apis/gan/index.html">GAN API > documentation</a> > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... > gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:69: > //Set up server > normally there is a space after // so would normally look like: > > // Set up server
Bump. If at all possible, I'd like to get this checked in by EOW. My internship ends then. Sorry for being a bit pushy. On 2011年09月09日 21:57:43, namos wrote: > Is there a doc that describes everything I need to do to check in the maven jar? > Like how to generate the md5/sha and xml? Also, do we need to include anything > but what the library generator gives in the jar file? > On 2011年09月09日 21:07:51, yanivi wrote: > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml > > File gan-json-oauth-sample/pom.xml (right): > > > > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml... > > gan-json-oauth-sample/pom.xml:10: > <artifactId>gan-json-oauth-sample</artifactId> > > This directory and artifactId MUST be called gan-cmdline-sample to follow our > > new naming convention. > > > > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/pom.xml... > > gan-json-oauth-sample/pom.xml:102: <groupId>com.google.apis-samples</groupId> > > Missing dependency on google-api-services-gan jar, which again is still not in > > our mavenrepo. This is a requirement for all of our samples, because one > cannot > > run the sample without the jar the sample depends on. > > > > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... > > File gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java > > (right): > > > > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... > > gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:17: > > import com.google.api.services.gan.Gan; > > when running mvn compile I get this: > > > > > /home/yanivi/hg/google-api-java-client/samples/default/gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:[17,34] > > package com.google.api.services.gan does not exist > > > > See my other comment. > > > > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... > > gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:41: > * > > See for GAN API documentation: https://code.google.com/apis/gan/index.html > > prefer regular HTML syntax in JavaDoc, e.g. > > > > See <a href="https://code.google.com/apis/gan/index.html">GAN API > > documentation</a> > > > > > http://codereview.appspot.com/4899045/diff/9001/gan-json-oauth-sample/src/com... > > gan-json-oauth-sample/src/com/google/api/client/sample/gan/GanSample.java:69: > > //Set up server > > normally there is a space after // so would normally look like: > > > > // Set up server
As we discussed, we don't want to publish this sample or the library jars yet. Please talk to your PM for other options.
Sounds good. Thanks for your help! On Tue, Sep 13, 2011 at 4:32 PM, <yanivi@google.com> wrote: > As we discussed, we don't want to publish this sample or the library > jars yet. Please talk to your PM for other options. > > > http://codereview.appspot.com/**4899045/<http://codereview.appspot.com/4899045/> > -- Ryan Oman | Software Engineer Intern | namos@google.com | 414-238-7849
One last quick question. The shared-cmdline-sample doesn't appear to be in the maven repo. Is there some way to use this dependency without manually checking out the samples repo and using "mvn install" on the shared-cmdline-sample? On Tue, Sep 13, 2011 at 4:34 PM, Ryan Oman <namos@google.com> wrote: > Sounds good. Thanks for your help! > > > On Tue, Sep 13, 2011 at 4:32 PM, <yanivi@google.com> wrote: > >> As we discussed, we don't want to publish this sample or the library >> jars yet. Please talk to your PM for other options. >> >> >> http://codereview.appspot.com/**4899045/<http://codereview.appspot.com/4899045/> >> > > > > -- > Ryan Oman | Software Engineer Intern | namos@google.com | 414-238-7849 > > -- Ryan Oman | Software Engineer Intern | namos@google.com | 414-238-7849
On Wed, Sep 14, 2011 at 11:03 AM, Ryan Oman <namos@google.com> wrote: > One last quick question. The shared-cmdline-sample doesn't appear to be in > the maven repo. Is there some way to use this dependency without manually > checking out the samples repo and using "mvn install" on the > shared-cmdline-sample? As the instructions say, that's the way to do it. We intentionally don't want this to be in the Maven repo because it is not a library. -- Yaniv > > On Tue, Sep 13, 2011 at 4:34 PM, Ryan Oman <namos@google.com> wrote: > >> Sounds good. Thanks for your help! >> >> >> On Tue, Sep 13, 2011 at 4:32 PM, <yanivi@google.com> wrote: >> >>> As we discussed, we don't want to publish this sample or the library >>> jars yet. Please talk to your PM for other options. >>> >>> >>> http://codereview.appspot.com/**4899045/<http://codereview.appspot.com/4899045/> >>> >> >> >> >> -- >> Ryan Oman | Software Engineer Intern | namos@google.com | 414-238-7849 >> >> > > > -- > Ryan Oman | Software Engineer Intern | namos@google.com | 414-238-7849 > >