|
|
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing code review comments #Patch Set 3 : Minor fix #
Total comments: 2
Patch Set 4 : Addressing code review comments #
Total comments: 2
Patch Set 5 : Feedback #MessagesTotal messages: 8
http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... File google-api-client-assembly/readme.html (right): http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:6: <p>High-level details about this library can be found at <a href="http://code.google.com/p/google-api-java-client">http://code.google.com/p/google-api-java-client</a> can we move this line to right after line 11 <h3>Resources</h3> so essentially this would be a subtitle? http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:9: <a href='LICENSE'>LICENSE</a> Does license need to be its own section? Should we instead move it to the "Dependencies and Licenses" section? http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:11: <h3>Resources</h3> "Overview" instead of "Resources"? http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:13: <li><a href='http://code.google.com/p/google-api-java-client/wiki/ReleaseNotes#Version_1.7.0-beta'>Release Notes</a></li> if possible, please use ${project.version} variable http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:15: <li><a href='http://code.google.com/p/google-api-java-client/wiki/DeveloperGuide'>Developer Guide</a></li> "Developer's Guide" http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:19: <h3>List of Dependencies and their Licenses</h3> maybe simplify to "Dependencies and Licenses"? http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:26: Dependent jars can be found in the <a href='dependencies'>dependencies</a> folder. similarly this line could act as a subtitle for "List of Dependencies and their Licenses" by moving to immediately after line 19. http://codereview.appspot.com/5694093/diff/1/release.html File release.html (right): http://codereview.appspot.com/5694093/diff/1/release.html#newcode98 release.html:98: google-api-client-assembly/readme.html (change "1.6" to "1.7") see my comment in the readme.html file to see if this is still needed here
Sign in to reply to this message.
http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... File google-api-client-assembly/readme.html (right): http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:6: <p>High-level details about this library can be found at <a href="http://code.google.com/p/google-api-java-client">http://code.google.com/p/google-api-java-client</a> On 2012/02/27 20:29:58, yanivi wrote: > can we move this line to right after line 11 <h3>Resources</h3> so essentially > this would be a subtitle? Done. http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:9: <a href='LICENSE'>LICENSE</a> On 2012/02/27 20:29:58, yanivi wrote: > Does license need to be its own section? Should we instead move it to the > "Dependencies and Licenses" section? Done. http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:11: <h3>Resources</h3> On 2012/02/27 20:29:58, yanivi wrote: > "Overview" instead of "Resources"? Done. http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:13: <li><a href='http://code.google.com/p/google-api-java-client/wiki/ReleaseNotes#Version_1.7.0-beta'>Release Notes</a></li> On 2012/02/27 20:29:58, yanivi wrote: > if possible, please use ${project.version} variable Done. I did not use it before because I saw SNAPSHOT in the project.version but that will not be there in the release. http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:15: <li><a href='http://code.google.com/p/google-api-java-client/wiki/DeveloperGuide'>Developer Guide</a></li> On 2012/02/27 20:29:58, yanivi wrote: > "Developer's Guide" Done. http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:19: <h3>List of Dependencies and their Licenses</h3> On 2012/02/27 20:29:58, yanivi wrote: > maybe simplify to "Dependencies and Licenses"? Done. http://codereview.appspot.com/5694093/diff/1/google-api-client-assembly/readm... google-api-client-assembly/readme.html:26: Dependent jars can be found in the <a href='dependencies'>dependencies</a> folder. On 2012/02/27 20:29:58, yanivi wrote: > similarly this line could act as a subtitle for "List of Dependencies and their > Licenses" by moving to immediately after line 19. Done. http://codereview.appspot.com/5694093/diff/1/release.html File release.html (right): http://codereview.appspot.com/5694093/diff/1/release.html#newcode98 release.html:98: google-api-client-assembly/readme.html (change "1.6" to "1.7") On 2012/02/27 20:29:58, yanivi wrote: > see my comment in the readme.html file to see if this is still needed here Reverted this change.
Sign in to reply to this message.
http://codereview.appspot.com/5694093/diff/5/google-api-client-assembly/readm... File google-api-client-assembly/readme.html (right): http://codereview.appspot.com/5694093/diff/5/google-api-client-assembly/readm... google-api-client-assembly/readme.html:19: <li><a href='dependencies/dependencies.html'>dependencies.html</a></li> My concern is that although it is obvious to you and me what these dependencies.html files are, it is entirely non-obvious to the average user. I think the biggest thing that's unclear is what the difference is between the 4 files. For example, we know that dependencies.html is an analysis of the dependency structure and licenses for google-api-client, android2-dependencies.html is the same for google-api-client-android2, etc. It would probably help to specify that here. That brings up another issue: we should probably explain what's going on in the root directory with all of these jars. I think most people when they download a library expect a binary jar and possibly a source jar. But instead they see 20+ jars. So the first question: should google-http-* and google-oauth-* even be in the root directory? They are really dependencies, not part of this library, even though you and I don't think about it that way :) Second, it would help to just explicitly say something like google-api-client-android2 is for Android SDK 2.0 & higher, google-api-client-appengine is for Google App Engine, google-api-client-servlet is for HTTP Servlet environment, and google-api-client is for all platforms, etc. I tried to make it as obvious as possible with the naming, but I think the more we just explain explicitly the less confused users will be. What do you think?
Sign in to reply to this message.
http://codereview.appspot.com/5694093/diff/5/google-api-client-assembly/readm... File google-api-client-assembly/readme.html (right): http://codereview.appspot.com/5694093/diff/5/google-api-client-assembly/readm... google-api-client-assembly/readme.html:19: <li><a href='dependencies/dependencies.html'>dependencies.html</a></li> On 2012/02/28 13:43:52, yanivi wrote: > My concern is that although it is obvious to you and me what these > dependencies.html files are, it is entirely non-obvious to the average user. I > think the biggest thing that's unclear is what the difference is between the 4 > files. For example, we know that dependencies.html is an analysis of the > dependency structure and licenses for google-api-client, > android2-dependencies.html is the same for google-api-client-android2, etc. It > would probably help to specify that here. Gave this a shot. > > That brings up another issue: we should probably explain what's going on in the > root directory with all of these jars. I think most people when they download a > library expect a binary jar and possibly a source jar. But instead they see 20+ > jars. > > So the first question: should google-http-* and google-oauth-* even be in the > root directory? They are really dependencies, not part of this library, even > though you and I don't think about it that way :) Yes they are dependencies, I cannot think of why they should not be in the dependencies folder. Done in assembly.xml. > > Second, it would help to just explicitly say something like > google-api-client-android2 is for Android SDK 2.0 & higher, > google-api-client-appengine is for Google App Engine, google-api-client-servlet > is for HTTP Servlet environment, and google-api-client is for all platforms, > etc. I tried to make it as obvious as possible with the naming, but I think the > more we just explain explicitly the less confused users will be. > > What do you think? Done.
Sign in to reply to this message.
http://codereview.appspot.com/5694093/diff/7001/google-api-client-assembly/as... File google-api-client-assembly/assembly.xml (left): http://codereview.appspot.com/5694093/diff/7001/google-api-client-assembly/as... google-api-client-assembly/assembly.xml:25: <exclude>google-*-client-*</exclude> well, we still want google-api-client-* in the root directory :) so I think you still want to keep this here and below, but just change it to "google-api-client-*"
Sign in to reply to this message.
http://codereview.appspot.com/5694093/diff/7001/google-api-client-assembly/as... File google-api-client-assembly/assembly.xml (left): http://codereview.appspot.com/5694093/diff/7001/google-api-client-assembly/as... google-api-client-assembly/assembly.xml:25: <exclude>google-*-client-*</exclude> On 2012/02/28 16:54:56, yanivi wrote: > well, we still want google-api-client-* in the root directory :) > so I think you still want to keep this here and below, but just change it to > "google-api-client-*" D'OH! Done.
Sign in to reply to this message.
|