Adding loggingEnabled to HttpRequest and HttpResponse

Fix for [Issue 98]: HttpRequest.setEnableLogging
Also created to support http://codereview.appspot.com/5539055/ : Adding Batch support to Google APIs

[yanivi, 2012-05-03 15:29:14]

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:126:
/** Determines whether logging should be enabled for this request. Defaults to
{@code true}. */
[optional] style: I think specifying the default value on a private field
declaration is overkill.  I can read the code just fine :)

similarly in HttpResponse

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:378:
* Defaults to {@code true}.
[optional] style: I would not specify the default in getter methods.  I think
it's more relevant to setter methods.

similarly in HttpResponse

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:113:
private boolean loggingEnabled = true;
remove the "= true" because that's misleading

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:401:
if (logger.isLoggable(Level.CONFIG)) {
you missed this one!

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/test/j...
File
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java
(right):

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/test/j...
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java:182:
subtestContentLoggingLimit("", 2, false);
add this case:
    subtestContentLoggingLimit("A", 2, false);

note that it fails!

[rmistry, 2012-05-03 15:36:50]

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:126:
/** Determines whether logging should be enabled for this request. Defaults to
{@code true}. */
On 2012/05/03 15:29:14, yanivi wrote:
> [optional] style: I think specifying the default value on a private field
> declaration is overkill.  I can read the code just fine :)
> 
> similarly in HttpResponse

Default is specified on almost all the fields in HttpRequest and HttpResponse.
I'll leave it for consistency.

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:378:
* Defaults to {@code true}.
On 2012/05/03 15:29:14, yanivi wrote:
> [optional] style: I would not specify the default in getter methods.  I think
> it's more relevant to setter methods.
> 
> similarly in HttpResponse

Same as above, leaving for consistency.

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java
(right):

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:113:
private boolean loggingEnabled = true;
On 2012/05/03 15:29:14, yanivi wrote:
> remove the "= true" because that's misleading

Done.

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:401:
if (logger.isLoggable(Level.CONFIG)) {
On 2012/05/03 15:29:14, yanivi wrote:
> you missed this one!

D'OH!

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/test/j...
File
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java
(right):

http://codereview.appspot.com/6174052/diff/2001/google-http-client/src/test/j...
google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java:182:
subtestContentLoggingLimit("", 2, false);
On 2012/05/03 15:29:14, yanivi wrote:
> add this case:
>     subtestContentLoggingLimit("A", 2, false);
> 
> note that it fails!

Added and it works now.

[yanivi, 2012-05-03 16:06:55]

LGTM