books-v1-json-cmdline-sample

[David Lu, 2011-05-11 05:23:05]

On 2011/05/11 05:20:50, David Lu wrote:

FYI: http://code.google.com/p/google-api-java-client/issues/detail?id=198

[aiuto, 2011-05-11 09:27:28]

First comment without seeing the code...

I think putting the source down
in src/main/java/com/google/api/client/sample/books/v1/cmdline...
to get the the long package name is inconvenient and misleading.  It implies
it is part c.g.api.client, rather than an example using it.  I would put the
single file at the same level as the pom.xml,
set the <sourceDirectory> to . and make the package name 'com.google.sample'

Also, do you really need the .classpath and .project files?

-tony

On Tue, May 10, 2011 at 10:20 PM, <dlu@google.com> wrote:

> Reviewers: aiuto,
>
>
>
> Please review this at http://codereview.appspot.com/4538048/
>
> Affected files:
>  A books-v1-json-cmdline-sample/.classpath
>  A books-v1-json-cmdline-sample/.project
>  A books-v1-json-cmdline-sample/.settings/org.eclipse.jdt.core.prefs
>  A books-v1-json-cmdline-sample/.settings/org.eclipse.jdt.ui.prefs
>  A books-v1-json-cmdline-sample/instructions.html
>  A books-v1-json-cmdline-sample/logging.properties
>  A books-v1-json-cmdline-sample/pom.xml
>  A
>
books-v1-json-cmdline-sample/src/main/java/com/google/api/client/sample/books/v1/cmdline/BooksSample.java
>
>
>

[David Lu, 2011-05-11 23:05:57]

I copied the buzz sample code for the package name.  I agree it's a bit long. 
I've trimmed it down as you suggested, but added a "books" for the project.

As for the .classpath and .project, that's another thing I copied from Buzz. 
The instructions.html file provide steps for importing the project into Eclipse,
which I think is useful.  However, I can remove the Eclipse artifacts if you
don't think the sample should be tied to a specific IDE.

On 2011/05/11 09:27:28, aiuto wrote:
> First comment without seeing the code...
> 
> I think putting the source down
> in src/main/java/com/google/api/client/sample/books/v1/cmdline...
> to get the the long package name is inconvenient and misleading.  It implies
> it is part c.g.api.client, rather than an example using it.  I would put the
> single file at the same level as the pom.xml,
> set the <sourceDirectory> to . and make the package name 'com.google.sample'
> 
> Also, do you really need the .classpath and .project files?
> 
> -tony
> 
> On Tue, May 10, 2011 at 10:20 PM, <mailto:dlu@google.com> wrote:
> 
> > Reviewers: aiuto,
> >
> >
> >
> > Please review this at http://codereview.appspot.com/4538048/
> >
> > Affected files:
> >  A books-v1-json-cmdline-sample/.classpath
> >  A books-v1-json-cmdline-sample/.project
> >  A books-v1-json-cmdline-sample/.settings/org.eclipse.jdt.core.prefs
> >  A books-v1-json-cmdline-sample/.settings/org.eclipse.jdt.ui.prefs
> >  A books-v1-json-cmdline-sample/instructions.html
> >  A books-v1-json-cmdline-sample/logging.properties
> >  A books-v1-json-cmdline-sample/pom.xml
> >  A
> >
>
books-v1-json-cmdline-sample/src/main/java/com/google/api/client/sample/books/v1/cmdline/BooksSample.java
> >
> >
> >

[David Lu, 2011-05-16 21:21:49]

PIng.

[aiuto, 2011-05-23 15:04:33]

lgtm

http://codereview.appspot.com/4538048/diff/14001/books-v1-json-cmdline-sample...
File books-v1-json-cmdline-sample/com/google/sample/books/BooksSample.java
(right):

http://codereview.appspot.com/4538048/diff/14001/books-v1-json-cmdline-sample...
books-v1-json-cmdline-sample/com/google/sample/books/BooksSample.java:95: if
(volumeInfo.description != null && volumeInfo.description.length() > 0) {
Do you need the length() tests too? It seems like if the description is being
returned and it is null, then something is wrong with the API def or the base
library. It should either be null or have some real content.

[David Lu, 2011-05-25 18:17:40]

http://codereview.appspot.com/4538048/diff/14001/books-v1-json-cmdline-sample...
File books-v1-json-cmdline-sample/com/google/sample/books/BooksSample.java
(right):

http://codereview.appspot.com/4538048/diff/14001/books-v1-json-cmdline-sample...
books-v1-json-cmdline-sample/com/google/sample/books/BooksSample.java:95: if
(volumeInfo.description != null && volumeInfo.description.length() > 0) {
We've seen at least one instance where the data is "".  So this prevents a blank
string from being shown.

On 2011/05/23 15:04:34, aiuto wrote:
> Do you need the length() tests too? It seems like if the description is being
> returned and it is null, then something is wrong with the API def or the base
> library. It should either be null or have some real content.

[aiuto, 2011-05-25 18:56:18]

lgtm

[aiuto, 2011-06-24 02:36:27]

On 2011/05/25 18:56:18, aiuto wrote:
> lgtm

David. Is this hg push'ed? You have to manually close out the issue after you
push because codereview is not connect to the repository.

[David Lu, 2011-06-24 02:58:38]

Yes it was.  I just closed it.  Please let me know if there's anything else
I need to do.  Thanks!

On Thu, Jun 23, 2011 at 7:36 PM, <aiuto@google.com> wrote:

> On 2011/05/25 18:56:18, aiuto wrote:
>
>> lgtm
>>
>
> David. Is this hg push'ed? You have to manually close out the issue
> after you push because codereview is not connect to the repository.
>
>
http://codereview.appspot.com/**4538048/<http://codereview.appspot.com/4538048/>
>