Issues 320, 324 and 260 - .NET 4.0, Google.Apis.Http namespace and NuGet

We wanted to created a PCL library (Issue 319), but unfortunately HttpClient PCL version has a problematic licensse (Take a look in http://nuget.org/packages/Microsoft.Net.Http)

So we only going to upgrade to .NET 4.0 and create Google.Apis.Http namespace which uses HttpClient.

In addition start supporting NuGet (Issue 260)

The follow manual + unit tests were made:

ETag collision (including Tasks.ETagCollision sample)
Media upload (a separate sample I wrote to upload media)
Generate the new library on Mono
Performance tests - running requests in loop

TODO (after pushing this CL):
- clone repostiory to check everything works in "a new client" --> DONE 
- add third party library with the right .net 4.0 dlls + licenses --> DONE
- update samples (https://codereview.appspot.com/9620044/)

[ngmiceli, 2013-05-14 20:35:02]

Comments so far after reviewing the HTTP namespace and tests, and the services
namespace and tests.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.Tests.Utili...
File Src/GoogleApis.Tests.Utility/CountableMessageHandler.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.Tests.Utili...
Src/GoogleApis.Tests.Utility/CountableMessageHandler.cs:26: }
It would be nice if this class held the logic for counting the calls, instead of
just being a wrapper for an int.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:287: public void
Test_Authentication_UnsccuessfulHandler()
Spelling :)

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:51: private const string
UserAgentSuffix = "google-api-dotnet-client/{0} (gzip)";
Put in the ApiVersion in the construction of the const, rather than calculating
it at use time

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:56: private readonly
IList<IHttpUnsuccessfulResponseHandler> unsuccessfulResponseHandlers =
I'm unsure about having a List of handlers, as we don't want to make it too easy
for the user to shoot themselves in the foot. What if they add a handler that
conflicts with one of the default handlers already in the list?

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:143: catch (Exception)
Catch the specific exception, and document why it would occur

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:162: try
I would remove the try-catch block. An exception here seems like a fair thing to
be fatal rather than just logged and ignored. Unless you expect an exception to
be thrown, in which case document and catch that specific exception.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:218: if (response != null
&& !response.IsSuccessStatusCode)
All the if's and elses are a little hard to read here. Could we reorganize and
nest a little neater to bring the logic closer?

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:287: }
Also check if the response still isn't successful and log that we ran out of
retries and we're still not at a 2XX

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:302: if
(message.IsRedirectStatusCode() && FollowRedirect && uri != null)
Switch to if not, or if not, or if not, return false. Then unindent the
following

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
File Src/GoogleApis/Apis/Http/HttpExtenstions.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:27: using Google.Apis.Services;
This feels out of place here. It would be nice if the Http namespace could be
closed

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:65: public static void
SetContent(this HttpRequestMessage request, object body, IClientService service)
Can we refactor out the setContent method? You can easily set the request
content using built in methods. This is useful for setting a Serialized content.
But the form of serializing depends on the Service, meaning this should be
somewhere in the request/service layer. 

Perhaps do 
service.setRequestSerializedContent(request, body) ?

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:76: // to return something which is
related to Http protocol.
I agree, but the first option seems to be the cleaner option. The serialization
determines the media type.

I would like if the Serializer knows what type its serializing things into. 

String format = serializer.getSerializationFormat()

That way the Serializer doesn't have HTTP logic in it, but has the information
needed after the serializer is set to also set the mediaType

request.setMediaType("application/" + format)

Or something like this. Just an idea.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:83:
content.Headers.Add("Content-Encoding", "gzip");
I would clear the headers before adding, as we know exactly what the
content-type and encoding are.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
File Src/GoogleApis/Apis/Services/BaseClientService.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:55: /// An Http client
initializer which is able to custom properties on
custom -> customize

Also elsewhere.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:64: /// <summary>
" "

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:176: var eTag =
response.Headers.ETag != null ? response.Headers.ETag.Tag : null;
Move this variable declaration closer to its usage

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:226: (result as
IDirectResponseSchema).ETag = eTag;
This doesn't feel like a part of "deserialization", but this is how it worked
before and I understand the desire to minimize changes in this changeset

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
File Src/GoogleApis/Apis/Services/IClientService.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/IClientService.cs:30: /// Client service contains
all the necessary information a Google service is required.
"all the necessary information that a Google service requires"

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/IClientService.cs:32: /// important properties like
API key, application name, Base Uri, etc.
Eyal wishes me to note that application name should be capital :)

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/IClientService.cs:63: /// <summary> Get the common
parameters information for this service. </summary>
Gets

[peleyal, 2013-05-15 13:51:02]

Waiting for the next session.

Thanks a lot Nick!

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.NET4/Google...
File Src/GoogleApis.NET4/GoogleApis.NET4.csproj (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.NET4/Google...
Src/GoogleApis.NET4/GoogleApis.NET4.csproj:11:
<AssemblyName>Google.Apis.NET4</AssemblyName>
Rename to Google.Apis.FullProfile (by David and Anash suggestion)

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.Tests.Utili...
File Src/GoogleApis.Tests.Utility/CountableMessageHandler.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.Tests.Utili...
Src/GoogleApis.Tests.Utility/CountableMessageHandler.cs:26: }
On 2013/05/14 20:35:02, ngmiceli wrote:
> It would be nice if this class held the logic for counting the calls, instead
of
> just being a wrapper for an int.

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:287: public void
Test_Authentication_UnsccuessfulHandler()
On 2013/05/14 20:35:02, ngmiceli wrote:
> Spelling :)

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:51: private const string
UserAgentSuffix = "google-api-dotnet-client/{0} (gzip)";
On 2013/05/14 20:35:02, ngmiceli wrote:
> Put in the ApiVersion in the construction of the const, rather than
calculating
> it at use time

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:56: private readonly
IList<IHttpUnsuccessfulResponseHandler> unsuccessfulResponseHandlers =
Good point :) I agree with you. But...
I added in the documentation which handlers we support by default (like handling
401 with refreshing the token).
Yes the user is able to misuse our API, but if the documentation is good enough
we will avoid that.
I added the right documentation in the declaration of  BaseClientService.
On 2013/05/14 20:35:02, ngmiceli wrote:
> I'm unsure about having a List of handlers, as we don't want to make it too
easy
> for the user to shoot themselves in the foot. What if they add a handler that
> conflicts with one of the default handlers already in the list?

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:143: catch (Exception)
I'll investigate it further when I'll have a Silverlight \ WP sample
On 2013/05/14 20:35:02, ngmiceli wrote:
> Catch the specific exception, and document why it would occur

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:162: try
On 2013/05/14 20:35:02, ngmiceli wrote:
> I would remove the try-catch block. An exception here seems like a fair thing
to
> be fatal rather than just logged and ignored. Unless you expect an exception
to
> be thrown, in which case document and catch that specific exception.

Done. Removed try-catch from all handlers+interceptors

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:218: if (response != null
&& !response.IsSuccessStatusCode)
On 2013/05/14 20:35:02, ngmiceli wrote:
> All the if's and elses are a little hard to read here. Could we reorganize and
> nest a little neater to bring the logic closer?

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:287: }
On 2013/05/14 20:35:02, ngmiceli wrote:
> Also check if the response still isn't successful and log that we ran out of
> retries and we're still not at a 2XX

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:302: if
(message.IsRedirectStatusCode() && FollowRedirect && uri != null)
On 2013/05/14 20:35:02, ngmiceli wrote:
> Switch to if not, or if not, or if not, return false. Then unindent the
> following

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
File Src/GoogleApis/Apis/Http/HttpExtenstions.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:27: using Google.Apis.Services;
On 2013/05/14 20:35:02, ngmiceli wrote:
> This feels out of place here. It would be nice if the Http namespace could be
> closed 

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:65: public static void
SetContent(this HttpRequestMessage request, object body, IClientService service)
It was moved to BaseClientService. Although maybe it should be a utility method
(what do you think? - let's discuss on that one...)
On 2013/05/14 20:35:02, ngmiceli wrote:
> Can we refactor out the setContent method? You can easily set the request
> content using built in methods. This is useful for setting a Serialized
content.
> But the form of serializing depends on the Service, meaning this should be
> somewhere in the request/service layer. 
> 
> Perhaps do 
> service.setRequestSerializedContent(request, body) ?

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:76: // to return something which is
related to Http protocol.
On 2013/05/14 20:35:02, ngmiceli wrote:
> I agree, but the first option seems to be the cleaner option. The
serialization
> determines the media type.
> 
> I would like if the Serializer knows what type its serializing things into. 
> 
> String format = serializer.getSerializationFormat()
> 
> That way the Serializer doesn't have HTTP logic in it, but has the information
> needed after the serializer is set to also set the mediaType
> 
> request.setMediaType("application/" + format)
> 
> Or something like this. Just an idea.

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:83:
content.Headers.Add("Content-Encoding", "gzip");
I think we dicuss on that one and decided that if the user set something, we
should not remove it.
I think that the code is clean now, with the change of using the right
properties - content.Headers.ContentEncoding and content.Headers.ContentType.
On 2013/05/14 20:35:02, ngmiceli wrote:
> I would clear the headers before adding, as we know exactly what the
> content-type and encoding are.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
File Src/GoogleApis/Apis/Services/BaseClientService.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:55: /// An Http client
initializer which is able to custom properties on
On 2013/05/14 20:35:02, ngmiceli wrote:
> custom -> customize
> 
> Also elsewhere.

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:64: /// <summary>
On 2013/05/14 20:35:02, ngmiceli wrote:
> " "

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:176: var eTag =
response.Headers.ETag != null ? response.Headers.ETag.Tag : null;
On 2013/05/14 20:35:02, ngmiceli wrote:
> Move this variable declaration closer to its usage

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:226: (result as
IDirectResponseSchema).ETag = eTag;
Added a todo...
On 2013/05/14 20:35:02, ngmiceli wrote:
> This doesn't feel like a part of "deserialization", but this is how it worked
> before and I understand the desire to minimize changes in this changeset

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
File Src/GoogleApis/Apis/Services/IClientService.cs (right):

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/IClientService.cs:30: /// Client service contains
all the necessary information a Google service is required.
On 2013/05/14 20:35:02, ngmiceli wrote:
> "all the necessary information that a Google service requires"

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/IClientService.cs:32: /// important properties like
API key, application name, Base Uri, etc.
On 2013/05/14 20:35:02, ngmiceli wrote:
> Eyal wishes me to note that application name should be capital :)

Done.

https://codereview.appspot.com/8596049/diff/243001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/IClientService.cs:63: /// <summary> Get the common
parameters information for this service. </summary>
On 2013/05/14 20:35:02, ngmiceli wrote:
> Gets

Done.

[ngmiceli, 2013-05-15 21:15:08]

Comments from the next round of review:

Major AI: Check out everything async. Confirm that we are actually asynchronous
and not blocking. Test these confirmations formally. 

Remaining: Review resumable upload & upload constructor decorator

https://codereview.appspot.com/8596049/diff/267006/GoogleApisClient.sln
File GoogleApisClient.sln (right):

https://codereview.appspot.com/8596049/diff/267006/GoogleApisClient.sln#newcode2
GoogleApisClient.sln:2: Microsoft Visual Studio Solution File, Format Version
12.00
I'm no good at manually reviewing things like solution files -- just empirically
test it by running this changeset on a different machine, like your personal
laptop.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile...
File Src/GoogleApis.FullProfile/Apis/Logging/ConsoleLogger.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile...
Src/GoogleApis.FullProfile/Apis/Logging/ConsoleLogger.cs:23: namespace
Google.Apis.Logging
Consider removing. We already have log4net

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile...
File Src/GoogleApis.FullProfile/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile...
Src/GoogleApis.FullProfile/Properties/AssemblyInfo.cs:29: [assembly:
AssemblyCopyright("Copyright � Google Inc 2013")]
Replace with ©. Similarly in other AssemblyInfo files.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Utils/EnumStringValueTypeConverterTest.cs (left):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Utils/EnumStringValueTypeConverterTest.cs:28: public
class EnumStringValueTypeConverterTest
Are these tests moved/replicated somewhere? The new logic that handles this
should be tested

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Strea...
File Src/GoogleApis.Tests/StreamDiscoveryDeviceTests.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Strea...
Src/GoogleApis.Tests/StreamDiscoveryDeviceTests.cs:19: namespace
Google.Apis.Tests
rm this pointless file :)

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tools.CodeG...
File Src/GoogleApis.Tools.CodeGen.Tests/CachedWebDiscoveryDeviceTests.cs
(right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tools.CodeG...
Src/GoogleApis.Tools.CodeGen.Tests/CachedWebDiscoveryDeviceTests.cs:87: var
device = new CachedWebDiscoveryDevice { DiscoveryUri = new
Uri("http://google.com/$#@!%^&*()[]+=/\\<>:;") };
Fix line length

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/C...
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:135: protected override
async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
We should have a test to specifically check that this is asynchronous

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:175: response = await
base.SendAsync(request, cancellationToken).ConfigureAwait(false);
We need to check that this is actually asynchronous and that the method isn't
blocking on the await

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs:104: public async
Task<TResponse> ExecuteAsync()
Add a test to confirm that the ExecuteAsync is in fact asynchronous and not
blocking

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs:106: using (var response =
await ExecuteUnparsed().ConfigureAwait(false))
I'm afraid that awaiting here will cause the Async method to block on a result,
causing it to not really be asynchronous to the user at all.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/ParameterCollection.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/ParameterCollection.cs:126: if (info.Length >= 2)
if info.Length != 2, shouldn't that be an error? 
Make sure we test this

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/RequestBuilder.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/RequestBuilder.cs:69: /// Default Value is <<see
cref="Google.Apis.Http.HttpConsts.Get"/>
Remove second < in <<see

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/RequestBuilder.cs:83: /// Construct a new HTTP web
request builder. The
The?? The what? I'll never know!

[peleyal, 2013-05-17 14:10:15]

Main change was downgrade HttpClient to an older version and stop supporting
PCL.

Hopefully, Microsoft will release the PCL version of HttpClient soon, so we will
be able to add PCL support back

https://codereview.appspot.com/8596049/diff/267006/GoogleApisClient.sln
File GoogleApisClient.sln (right):

https://codereview.appspot.com/8596049/diff/267006/GoogleApisClient.sln#newcode2
GoogleApisClient.sln:2: Microsoft Visual Studio Solution File, Format Version
12.00
Maybe because this CL is huge, I wan't able to import it on other clone env.
I added a todo, after committing this to confirm it works when cloning the
repository. Is it good enough?
On 2013/05/15 21:15:08, ngmiceli wrote:
> I'm no good at manually reviewing things like solution files -- just
empirically
> test it by running this changeset on a different machine, like your personal
> laptop.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile...
File Src/GoogleApis.FullProfile/Apis/Logging/ConsoleLogger.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile...
Src/GoogleApis.FullProfile/Apis/Logging/ConsoleLogger.cs:23: namespace
Google.Apis.Logging
On 2013/05/15 21:15:08, ngmiceli wrote:
> Consider removing. We already have log4net

Done.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile...
File Src/GoogleApis.FullProfile/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile...
Src/GoogleApis.FullProfile/Properties/AssemblyInfo.cs:29: [assembly:
AssemblyCopyright("Copyright � Google Inc 2013")]
I changed it back to the right symbol and opened a bug
https://code.google.com/p/google-api-dotnet-client/issues/detail?id=323
On 2013/05/15 21:15:08, ngmiceli wrote:
> Replace with ©. Similarly in other AssemblyInfo files.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Utils/EnumStringValueTypeConverterTest.cs (left):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Utils/EnumStringValueTypeConverterTest.cs:28: public
class EnumStringValueTypeConverterTest
Done.
Most of the tests here are a converter tests. I checked that the logic of
converter an enum to string works as expected on UtilitiesTests

On 2013/05/15 21:15:08, ngmiceli wrote:
> Are these tests moved/replicated somewhere? The new logic that handles this
> should be tested

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Strea...
File Src/GoogleApis.Tests/StreamDiscoveryDeviceTests.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Strea...
Src/GoogleApis.Tests/StreamDiscoveryDeviceTests.cs:19: namespace
Google.Apis.Tests
On 2013/05/15 21:15:08, ngmiceli wrote:
> rm this pointless file :)

Done.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tools.CodeG...
File Src/GoogleApis.Tools.CodeGen.Tests/CachedWebDiscoveryDeviceTests.cs
(right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tools.CodeG...
Src/GoogleApis.Tools.CodeGen.Tests/CachedWebDiscoveryDeviceTests.cs:87: var
device = new CachedWebDiscoveryDevice { DiscoveryUri = new
Uri("http://google.com/$#@!%^&*()[]+=/\\<>:;") };
On 2013/05/15 21:15:08, ngmiceli wrote:
> Fix line length

Done.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/C...
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:135: protected override
async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
I added tests that our ExecuteAsync and ExecuteAsStreamAsync are async. We
inherit that method from the base message handler, so I'm not going to check
this one.
On 2013/05/15 21:15:08, ngmiceli wrote:
> We should have a test to specifically check that this is asynchronous

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:175: response = await
base.SendAsync(request, cancellationToken).ConfigureAwait(false);
ditto
On 2013/05/15 21:15:08, ngmiceli wrote:
> We need to check that this is actually asynchronous and that the method isn't
> blocking on the await

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs:104: public async
Task<TResponse> ExecuteAsync()
On 2013/05/15 21:15:08, ngmiceli wrote:
> Add a test to confirm that the ExecuteAsync is in fact asynchronous and not
> blocking

Done.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs:106: using (var response =
await ExecuteUnparsed().ConfigureAwait(false))
I changed the mechanism and tested it. Take a look!
On 2013/05/15 21:15:08, ngmiceli wrote:
> I'm afraid that awaiting here will cause the Async method to block on a
result,
> causing it to not really be asynchronous to the user at all.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/ParameterCollection.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/ParameterCollection.cs:126: if (info.Length >= 2)
On 2013/05/15 21:15:08, ngmiceli wrote:
> if info.Length != 2, shouldn't that be an error? 
> Make sure we test this

Done.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/RequestBuilder.cs (right):

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/RequestBuilder.cs:69: /// Default Value is <<see
cref="Google.Apis.Http.HttpConsts.Get"/>
On 2013/05/15 21:15:08, ngmiceli wrote:
> Remove second < in <<see

Done.

https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/RequestBuilder.cs:83: /// Construct a new HTTP web
request builder. The
On 2013/05/15 21:15:08, ngmiceli wrote:
> The?? The what? I'll never know!

I'll explain to you when you will be old enough :)

[peleyal, 2013-05-17 16:31:57]

Hi David,

I know that this CL is huge (and by huge I mean huggggggggge).
Nick helped so much in reviewing this CL, Thanks again Nick!
If you want to take a look, I think that you should explore only the
following files:

Configurable Http Client -
https://codereview.appspot.com/8596049/patch/330001/331085
Configurable Message Handler -
https://codereview.appspot.com/8596049/patch/330001/331086
Client Service Request -
https://codereview.appspot.com/8596049/patch/330001/331101
Base Client Service -
https://codereview.appspot.com/8596049/patch/330001/331113

Thanks and have a great weekend!
Eyal



On Fri, May 17, 2013 at 10:10 AM, <peleyal@google.com> wrote:

> Main change was downgrade HttpClient to an older version and stop
> supporting PCL.
>
> Hopefully, Microsoft will release the PCL version of HttpClient soon, so
> we will be able to add PCL support back
>
>
> https://codereview.appspot.**com/8596049/diff/267006/**
>
GoogleApisClient.sln<https://codereview.appspot.com/8596049/diff/267006/GoogleApisClient.sln>
> File GoogleApisClient.sln (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/**
>
GoogleApisClient.sln#newcode2<https://codereview.appspot.com/8596049/diff/267006/GoogleApisClient.sln#newcode2>
> GoogleApisClient.sln:2: Microsoft Visual Studio Solution File, Format
> Version 12.00
> Maybe because this CL is huge, I wan't able to import it on other clone
> env.
> I added a todo, after committing this to confirm it works when cloning
> the repository. Is it good enough?
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> I'm no good at manually reviewing things like solution files -- just
>>
> empirically
>
>> test it by running this changeset on a different machine, like your
>>
> personal
>
>> laptop.
>>
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis.FullProfile/Apis/**Logging/ConsoleLogger.cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile/Apis/Logging/ConsoleLogger.cs>
> File Src/GoogleApis.FullProfile/**Apis/Logging/ConsoleLogger.cs (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis.FullProfile/Apis/**Logging/ConsoleLogger.cs#**newcode23<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile/Apis/Logging/ConsoleLogger.cs#newcode23>
> Src/GoogleApis.FullProfile/**Apis/Logging/ConsoleLogger.cs:**23: namespace
> Google.Apis.Logging
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> Consider removing. We already have log4net
>>
>
> Done.
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis.FullProfile/**Properties/AssemblyInfo.cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile/Properties/AssemblyInfo.cs>
> File Src/GoogleApis.FullProfile/**Properties/AssemblyInfo.cs (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis.FullProfile/**Properties/AssemblyInfo.cs#**newcode29<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.FullProfile/Properties/AssemblyInfo.cs#newcode29>
> Src/GoogleApis.FullProfile/**Properties/AssemblyInfo.cs:29: [assembly:
> AssemblyCopyright("Copyright � Google Inc 2013")]
> I changed it back to the right symbol and opened a bug
> https://code.google.com/p/**google-api-dotnet-client/**
>
issues/detail?id=323<https://code.google.com/p/google-api-dotnet-client/issues/detail?id=323>
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> Replace with ©. Similarly in other AssemblyInfo files.
>>
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis.Tests/Apis/Utils/**EnumStringValueTypeConverterTe**st.cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Apis/Utils/EnumStringValueTypeConverterTest.cs>
> File Src/GoogleApis.Tests/Apis/**Utils/**EnumStringValueTypeConverterTe**
> st.cs
> (left):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
> GoogleApis.Tests/Apis/Utils/**EnumStringValueTypeConverterTe**
>
st.cs#oldcode28<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/Apis/Utils/EnumStringValueTypeConverterTest.cs#oldcode28>
> Src/GoogleApis.Tests/Apis/**Utils/**EnumStringValueTypeConverterTe**
> st.cs:28:
> public class EnumStringValueTypeConverterTe**st
> Done.
> Most of the tests here are a converter tests. I checked that the logic
> of converter an enum to string works as expected on UtilitiesTests
>
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> Are these tests moved/replicated somewhere? The new logic that handles
>>
> this
>
>> should be tested
>>
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis.Tests/**StreamDiscoveryDeviceTests.cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/StreamDiscoveryDeviceTests.cs>
> File Src/GoogleApis.Tests/**StreamDiscoveryDeviceTests.cs (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis.Tests/**StreamDiscoveryDeviceTests.cs#**newcode19<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tests/StreamDiscoveryDeviceTests.cs#newcode19>
> Src/GoogleApis.Tests/**StreamDiscoveryDeviceTests.cs:**19: namespace
> Google.Apis.Tests
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> rm this pointless file :)
>>
>
> Done.
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis.Tools.CodeGen.**Tests/**CachedWebDiscoveryDeviceTests.**cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tools.CodeGen.Tests/CachedWebDiscoveryDeviceTests.cs>
> File Src/GoogleApis.Tools.CodeGen.**Tests/**CachedWebDiscoveryDeviceTests.
> **cs
> (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
> GoogleApis.Tools.CodeGen.**Tests/**CachedWebDiscoveryDeviceTests.**
>
cs#newcode87<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis.Tools.CodeGen.Tests/CachedWebDiscoveryDeviceTests.cs#newcode87>
> Src/GoogleApis.Tools.CodeGen.**Tests/**CachedWebDiscoveryDeviceTests.**
> cs:87:
> var device = new CachedWebDiscoveryDevice { DiscoveryUri = new
> Uri("http://google.com/$#@!%^&***()[]+=/\\<>:;") };
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> Fix line length
>>
>
> Done.
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Http/**ConfigurableMessageHandler.cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs>
> File Src/GoogleApis/Apis/Http/**ConfigurableMessageHandler.cs (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Http/**ConfigurableMessageHandler.cs#**newcode135<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs#newcode135>
> Src/GoogleApis/Apis/Http/**ConfigurableMessageHandler.cs:**135: protected
> override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
> request,
> I added tests that our ExecuteAsync and ExecuteAsStreamAsync are async.
> We inherit that method from the base message handler, so I'm not going
> to check this one.
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> We should have a test to specifically check that this is asynchronous
>>
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Http/**ConfigurableMessageHandler.cs#**newcode175<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs#newcode175>
> Src/GoogleApis/Apis/Http/**ConfigurableMessageHandler.cs:**175: response =
> await base.SendAsync(request, cancellationToken).**ConfigureAwait(false);
> ditto
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> We need to check that this is actually asynchronous and that the
>>
> method isn't
>
>> blocking on the await
>>
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Requests/**ClientServiceRequest.cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs>
> File Src/GoogleApis/Apis/Requests/**ClientServiceRequest.cs (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Requests/**ClientServiceRequest.cs#**newcode104<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs#newcode104>
> Src/GoogleApis/Apis/Requests/**ClientServiceRequest.cs:104: public async
> Task<TResponse> ExecuteAsync()
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> Add a test to confirm that the ExecuteAsync is in fact asynchronous
>>
> and not
>
>> blocking
>>
>
> Done.
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Requests/**ClientServiceRequest.cs#**newcode106<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs#newcode106>
> Src/GoogleApis/Apis/Requests/**ClientServiceRequest.cs:106: using (var
> response = await ExecuteUnparsed().**ConfigureAwait(false))
> I changed the mechanism and tested it. Take a look!
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> I'm afraid that awaiting here will cause the Async method to block on
>>
> a result,
>
>> causing it to not really be asynchronous to the user at all.
>>
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Requests/**ParameterCollection.cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Requests/ParameterCollection.cs>
> File Src/GoogleApis/Apis/Requests/**ParameterCollection.cs (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Requests/**ParameterCollection.cs#**newcode126<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Requests/ParameterCollection.cs#newcode126>
> Src/GoogleApis/Apis/Requests/**ParameterCollection.cs:126: if (info.Length
>
>> = 2)
>>
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> if info.Length != 2, shouldn't that be an error?
>> Make sure we test this
>>
>
> Done.
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Requests/**RequestBuilder.cs<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Requests/RequestBuilder.cs>
> File Src/GoogleApis/Apis/Requests/**RequestBuilder.cs (right):
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Requests/**RequestBuilder.cs#newcode69<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Requests/RequestBuilder.cs#newcode69>
> Src/GoogleApis/Apis/Requests/**RequestBuilder.cs:69: /// Default Value is
> <<see cref="Google.Apis.Http.**HttpConsts.Get"/>
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> Remove second < in <<see
>>
>
> Done.
>
> https://codereview.appspot.**com/8596049/diff/267006/Src/**
>
GoogleApis/Apis/Requests/**RequestBuilder.cs#newcode83<https://codereview.appspot.com/8596049/diff/267006/Src/GoogleApis/Apis/Requests/RequestBuilder.cs#newcode83>
> Src/GoogleApis/Apis/Requests/**RequestBuilder.cs:83: /// Construct a new
> HTTP web request builder. The
> On 2013/05/15 21:15:08, ngmiceli wrote:
>
>> The?? The what? I'll never know!
>>
>
> I'll explain to you when you will be old enough :)
>
>
https://codereview.appspot.**com/8596049/<https://codereview.appspot.com/8596...
>

[peleyal, 2013-05-17 21:17:24]

Last review?

https://codereview.appspot.com/8596049/diff/330001/Src/GoogleApis/Apis/Upload...
File Src/GoogleApis/Apis/Upload/ResumableUpload.cs (left):

https://codereview.appspot.com/8596049/diff/330001/Src/GoogleApis/Apis/Upload...
Src/GoogleApis/Apis/Upload/ResumableUpload.cs:75: private const string ErrorKey
= "error";
Eyal and Nick: we need to add back deserialize error in case we got unsuccessful
response. somehow I (Eyal) thought that it should be done by our message
handler, but actually the one that parses the error is ClientServiceRequest!

DONE.

https://codereview.appspot.com/8596049/diff/330001/Src/GoogleApis/Apis/Upload...
File Src/GoogleApis/Apis/Upload/ResumableUpload.cs (right):

https://codereview.appspot.com/8596049/diff/330001/Src/GoogleApis/Apis/Upload...
Src/GoogleApis/Apis/Upload/ResumableUpload.cs:340: UpdateProgress(new
ResumableUploadProgress(ex, BytesSent));
Eyal and Nick: add log here

DONE.

[peleyal, 2013-05-20 13:35:44]

Hi Nick,
I think that's going to be the finale of this CL!

You should compare patch 22 to the latest and I've a great feeling that your are
going to LGTM this.

Thanks,
Eyal

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Http/C...
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:32: /// This message
handler contains the main logic of our HTTP requests. It contains a list of
HTTP -> Http

DONE.

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:45: internal const int
MaxNumTries = 20;
Rename to MaxAllowedNumTries

DONE.

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:159: // intercept the
request
Review with Yaniv: rethink if we should implement our interceptors at delegate
handlers!

Added comment

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:216: // TODO(peleyal):
implement BackOffUnsucessfulHandler
Take a look in:
http://msdn.microsoft.com/en-us/library/hh680934(v=pandp.50).aspx for
Exponential Back Off

DONE.

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Http/H...
File Src/GoogleApis/Apis/Http/HttpClientFactory.cs (right):

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpClientFactory.cs:57: {
consider writing to log here if SupportedRedirectionConfiguration is true and
false....

DONE.

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpClientFactory.cs:63: {
ditto

DONE.

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs (right):

https://codereview.appspot.com/8596049/diff/335008/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/ClientServiceRequest.cs:137: // create a new task
completion source and return its task. In additional task we actually send the
request
Review with Yaniv: We don't need to create a new Task. Eyal: It didn't work me
in async way without doing so.
I needed to create a task here, so I can do the following:
....
var t = ExecuteAsync()
Console.WriteLine("HELLO WORLD");
t.ContinueWith(....)

[ngmiceli, 2013-05-22 12:58:58]

LGTM.

Congrats on achieving this huge improvement! This is a great change and a big
step forward for the .Net client library. Awesome work!

[peleyal, 2013-05-22 23:02:10]

!!!!WAS PUSHED!!!
I'm really excited about this change.

Thanks Nick :)

[anash, 2013-05-22 23:21:35]

Congrats!


On Thu, May 23, 2013 at 4:32 AM, <peleyal@google.com> wrote:

> !!!!WAS PUSHED!!!
> I'm really excited about this change.
>
> Thanks Nick :)
>
>
https://codereview.appspot.**com/8596049/<https://codereview.appspot.com/8596...
>



-- 
Anash P. Oommen | Developer Programs Engineer | anash@google.com |
 970-361-7311