http: proxy for some classes from Guava

http issue 140: Limit use of Guava library
https://code.google.com/p/google-http-java-client/issues/detail?id=140

This changeset makes a limited proxy for some classes from Guava: Joiner, Lists, Maps, Objects, Preconditions, Strings, Throwables

and makes a limited copy for some classes from Guava: Charsets, ByteStreams

[ngmiceli, 2013-01-30 00:24:08]

LGTM

Thought regarding documentation below:

https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/ja...
File google-http-client/src/main/java/com/google/api/client/util/Charsets.java
(right):

https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/Charsets.java:20: *
Copy for the Guava implementation of {@link com.google.common.base.Charsets}.
[optional] This explanation is a little strange, IMO. It makes perfect sense to
me, but I can't help but feel it's not terribly useful to a developer. Perhaps a
link to Guava's implementation would help. That way, it clarifies what Guava is
for those who don't know, but more importantly this javadoc doesn't actually
tell anything about the class itself. A link to their class or the docs for
their class will at least give the user an easy means to find the "real" docs
for this class.

Another clarifying solution might be better, this is just what came to mind.

Similarly for the other applicable (proxy) classes.

[yanivi, 2013-01-30 14:19:41]

Updated the JavaDoc.  Also reverted changes to the tests where it is okay to
allow Guava use.

https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/ja...
File google-http-client/src/main/java/com/google/api/client/util/Charsets.java
(right):

https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/Charsets.java:20: *
Copy for the Guava implementation of {@link com.google.common.base.Charsets}.
On 2013/01/30 00:24:08, ngmiceli wrote:
> [optional] This explanation is a little strange, IMO. It makes perfect sense
to
> me, but I can't help but feel it's not terribly useful to a developer. Perhaps
a
> link to Guava's implementation would help. That way, it clarifies what Guava
is
> for those who don't know, but more importantly this javadoc doesn't actually
> tell anything about the class itself. A link to their class or the docs for
> their class will at least give the user an easy means to find the "real" docs
> for this class.
> 
> Another clarifying solution might be better, this is just what came to mind.
> 
> Similarly for the other applicable (proxy) classes. 

Done.

[yanivi, 2013-01-30 14:20:27]

(please take another quick look)

[peleyal, 2013-01-30 15:53:00]

Why do you prefer this inconsistency (test uses guava while the src uses
the proxy classes)?
For me it looks that better solution is to change the tests as well.




On Wed, Jan 30, 2013 at 9:19 AM, <yanivi@google.com> wrote:

> Updated the JavaDoc.  Also reverted changes to the tests where it is
> okay to allow Guava use.
>
>
>
> https://codereview.appspot.**com/7230044/diff/92/google-**
>
http-client/src/main/java/com/**google/api/client/util/**Charsets.java<https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/java/com/google/api/client/util/Charsets.java>
> File
> google-http-client/src/main/**java/com/google/api/client/**
> util/Charsets.java
> (right):
>
> https://codereview.appspot.**com/7230044/diff/92/google-**
> http-client/src/main/java/com/**google/api/client/util/**
>
Charsets.java#newcode20<https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/java/com/google/api/client/util/Charsets.java#newcode20>
> google-http-client/src/main/**java/com/google/api/client/**
> util/Charsets.java:20:
> * Copy for the Guava implementation of {@link
> com.google.common.base.**Charsets}.
> On 2013/01/30 00:24:08, ngmiceli wrote:
>
>> [optional] This explanation is a little strange, IMO. It makes perfect
>>
> sense to
>
>> me, but I can't help but feel it's not terribly useful to a developer.
>>
> Perhaps a
>
>> link to Guava's implementation would help. That way, it clarifies what
>>
> Guava is
>
>> for those who don't know, but more importantly this javadoc doesn't
>>
> actually
>
>> tell anything about the class itself. A link to their class or the
>>
> docs for
>
>> their class will at least give the user an easy means to find the
>>
> "real" docs
>
>> for this class.
>>
>
>  Another clarifying solution might be better, this is just what came to
>>
> mind.
>
>  Similarly for the other applicable (proxy) classes.
>>
>
> Done.
>
>
https://codereview.appspot.**com/7230044/<https://codereview.appspot.com/7230...
>

[yanivi, 2013-01-30 16:42:39]

For example we can use com.google.common.collect freely in tests, whereas
we cannot in the regular library because it is too heavy-weight for
Android.  It is also less work because it reduces the number of proxies we
need to build.  So in particular TypeToken is only needed for tests and I
didn't want to bother making a proxy for it in the library.

And just to state the most important point: we expect src packages to be
dependencies for other code, whereas we expect test packages to not be
dependencies for other code.

Does that help explain my reasoning why src should be treated differently
from test?


On Wed, Jan 30, 2013 at 10:52 AM, Eyal Peled <peleyal@google.com> wrote:

> Why do you prefer this inconsistency (test uses guava while the src uses
> the proxy classes)?
> For me it looks that better solution is to change the tests as well.
>
>
>
>
> On Wed, Jan 30, 2013 at 9:19 AM, <yanivi@google.com> wrote:
>
>> Updated the JavaDoc.  Also reverted changes to the tests where it is
>> okay to allow Guava use.
>>
>>
>>
>> https://codereview.appspot.**com/7230044/diff/92/google-**
>>
http-client/src/main/java/com/**google/api/client/util/**Charsets.java<https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/java/com/google/api/client/util/Charsets.java>
>> File
>> google-http-client/src/main/**java/com/google/api/client/**
>> util/Charsets.java
>> (right):
>>
>> https://codereview.appspot.**com/7230044/diff/92/google-**
>> http-client/src/main/java/com/**google/api/client/util/**
>>
Charsets.java#newcode20<https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/java/com/google/api/client/util/Charsets.java#newcode20>
>> google-http-client/src/main/**java/com/google/api/client/**
>> util/Charsets.java:20:
>> * Copy for the Guava implementation of {@link
>> com.google.common.base.**Charsets}.
>> On 2013/01/30 00:24:08, ngmiceli wrote:
>>
>>> [optional] This explanation is a little strange, IMO. It makes perfect
>>>
>> sense to
>>
>>> me, but I can't help but feel it's not terribly useful to a developer.
>>>
>> Perhaps a
>>
>>> link to Guava's implementation would help. That way, it clarifies what
>>>
>> Guava is
>>
>>> for those who don't know, but more importantly this javadoc doesn't
>>>
>> actually
>>
>>> tell anything about the class itself. A link to their class or the
>>>
>> docs for
>>
>>> their class will at least give the user an easy means to find the
>>>
>> "real" docs
>>
>>> for this class.
>>>
>>
>>  Another clarifying solution might be better, this is just what came to
>>>
>> mind.
>>
>>  Similarly for the other applicable (proxy) classes.
>>>
>>
>> Done.
>>
>>
https://codereview.appspot.**com/7230044/<https://codereview.appspot.com/7230...
>>
>
>

[peleyal, 2013-01-30 16:50:40]

Yes it makes sense.

I didn't know that the tests use other classes, which it will take more
time to create proxies for them.

Thanks
On Jan 30, 2013 8:42 AM, "Yaniv Inbar (יניב ענבר)" <yanivi@google.com>
wrote:

> For example we can use com.google.common.collect freely in tests, whereas
> we cannot in the regular library because it is too heavy-weight for
> Android.  It is also less work because it reduces the number of proxies we
> need to build.  So in particular TypeToken is only needed for tests and I
> didn't want to bother making a proxy for it in the library.
>
> And just to state the most important point: we expect src packages to be
> dependencies for other code, whereas we expect test packages to not be
> dependencies for other code.
>
> Does that help explain my reasoning why src should be treated differently
> from test?
>
>
> On Wed, Jan 30, 2013 at 10:52 AM, Eyal Peled <peleyal@google.com> wrote:
>
>> Why do you prefer this inconsistency (test uses guava while the src uses
>> the proxy classes)?
>> For me it looks that better solution is to change the tests as well.
>>
>>
>>
>>
>> On Wed, Jan 30, 2013 at 9:19 AM, <yanivi@google.com> wrote:
>>
>>> Updated the JavaDoc.  Also reverted changes to the tests where it is
>>> okay to allow Guava use.
>>>
>>>
>>>
>>> https://codereview.appspot.**com/7230044/diff/92/google-**
>>>
http-client/src/main/java/com/**google/api/client/util/**Charsets.java<https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/java/com/google/api/client/util/Charsets.java>
>>> File
>>> google-http-client/src/main/**java/com/google/api/client/**
>>> util/Charsets.java
>>> (right):
>>>
>>> https://codereview.appspot.**com/7230044/diff/92/google-**
>>> http-client/src/main/java/com/**google/api/client/util/**
>>>
Charsets.java#newcode20<https://codereview.appspot.com/7230044/diff/92/google-http-client/src/main/java/com/google/api/client/util/Charsets.java#newcode20>
>>> google-http-client/src/main/**java/com/google/api/client/**
>>> util/Charsets.java:20:
>>> * Copy for the Guava implementation of {@link
>>> com.google.common.base.**Charsets}.
>>> On 2013/01/30 00:24:08, ngmiceli wrote:
>>>
>>>> [optional] This explanation is a little strange, IMO. It makes perfect
>>>>
>>> sense to
>>>
>>>> me, but I can't help but feel it's not terribly useful to a developer.
>>>>
>>> Perhaps a
>>>
>>>> link to Guava's implementation would help. That way, it clarifies what
>>>>
>>> Guava is
>>>
>>>> for those who don't know, but more importantly this javadoc doesn't
>>>>
>>> actually
>>>
>>>> tell anything about the class itself. A link to their class or the
>>>>
>>> docs for
>>>
>>>> their class will at least give the user an easy means to find the
>>>>
>>> "real" docs
>>>
>>>> for this class.
>>>>
>>>
>>>  Another clarifying solution might be better, this is just what came to
>>>>
>>> mind.
>>>
>>>  Similarly for the other applicable (proxy) classes.
>>>>
>>>
>>> Done.
>>>
>>>
https://codereview.appspot.**com/7230044/<https://codereview.appspot.com/7230...
>>>
>>
>>
>

[ngmiceli, 2013-01-31 22:43:44]

Just a few nits

https://codereview.appspot.com/7230044/diff/8004/google-http-client/src/main/...
File google-http-client/src/main/java/com/google/api/client/util/Joiner.java
(right):

https://codereview.appspot.com/7230044/diff/8004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/Joiner.java:21: *
{@link Map}) with a separator.
Use {@link java.util.Map Map} to avoid the unnecessary import
Similarly for other classes

https://codereview.appspot.com/7230044/diff/8004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/Joiner.java:30:
public final class Joiner {
This class is missing the private default constructor

https://codereview.appspot.com/7230044/diff/8004/google-http-client/src/main/...
File google-http-client/src/main/java/com/google/api/client/util/Lists.java
(right):

https://codereview.appspot.com/7230044/diff/8004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/Lists.java:18:
import java.util.List;
Remove unused import (by fixing javadoc)

[yanivi, 2013-01-31 23:24:51]

The imports *are* used.  They are used in the JavaDoc.  It is legal to do it,
and convenient because it allows you to avoid fully-qualified names in the
JavaDoc.

[ngmiceli, 2013-01-31 23:29:16]

LGTM after fixing missing private constructor from last group of comments

[yanivi, 2013-01-31 23:52:11]

https://codereview.appspot.com/7230044/diff/8004/google-http-client/src/main/...
File google-http-client/src/main/java/com/google/api/client/util/Joiner.java
(right):

https://codereview.appspot.com/7230044/diff/8004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/Joiner.java:30:
public final class Joiner {
On 2013/01/31 22:43:44, ngmiceli wrote:
> This class is missing the private default constructor

there's already a private constructor, so no need for a default one.