Reimplement OAuth2 library - Step 1

Reimplement OAuth2 library - Step 1

---sumnmary----
1. s/http/HTTP
2. Update to Bcl.Build 1.0.10 - no need to add the new targets file. The build will fail on the first time, and succeed in the second time.
3. Support Tasks in our unsuccessful response handler, I/O exception handler and interceptor
4. Move ExponentialBackOffInitializer to its own file
5. Create Parameters namespace (in requests)

[peleyal, 2013-09-10 01:14:44]

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Authenticati...
File Src/GoogleApis.Authentication.OAuth2/OAuth2Authenticator.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Authenticati...
Src/GoogleApis.Authentication.OAuth2/OAuth2Authenticator.cs:26: using
System.Threading.Tasks;
Fix using order

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
File Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:27: /// File data store
that implements <see cref="IDataStore"/>. This store creates a different file
for each
<seealso?

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:37: /// Constructs a new
file data store with the specified folder. This folder is going to be created
(if it's not
is created

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:51: /// Stores the given
value for the given key. It creates a new file (named <see
cref="GetStoredKey"/>) in
change GetStoredKey to be static public

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:124: ///
<returns></returns>
remove returns statement

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
File Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:254: ///
<summary> A message handler which returns a HTTP response message or throw an
exception. </summary>
remove space at the end of the comment

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
File Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterCollectionTest.cs
(right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterCollectionTest.cs:23:
using Google.Apis.Requests.Parameters;
reorder using

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
File Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterValidatorTest.cs
(right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterValidatorTest.cs:24:
using Google.Apis.Requests.Parameters;
reorder usings

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/[Mock]...
File Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/[Mock]...
Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs:23: using
System.Threading.Tasks;
reorder usings

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Http/Ba...
File Src/GoogleApis/Apis/Http/BackOffHandler.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Http/Ba...
Src/GoogleApis/Apis/Http/BackOffHandler.cs:56: /// The  default is <see
cref="DefaultHandleExceptionFunc"/>.
remove extra space
The default (single space)

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Http/Ba...
Src/GoogleApis/Apis/Http/BackOffHandler.cs:136: await Handle(args.SupportsRetry,
args.CurrentFailedTry, args.CancellationToken);
rename HandleAsync

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
File Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:27: public static
class ParameterUtils
add comment

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:31: /// input request.
It uses reflection to look at all properties marked with
to get all properties with the ... attribute

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:45: /// Creates a
parameter dictionary by using reflection to look at all properties marked with
look --> get all

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:59: /// Creates a
parameter dictionary by using reflection to look at all properties marked with
ditto

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:70: private static
void IterateParameters(object request, Action<RequestParameterType, string,
object> action)
add comment

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
File Src/GoogleApis/Apis/Requests/RequestBuilder.cs (left):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/RequestBuilder.cs:28: /// <summary> Utility class
used to build a request from the various parts of a REST Method call. </summary>
explain further.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
File Src/GoogleApis/Apis/Util/Store/IDataStore.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
Src/GoogleApis/Apis/Util/Store/IDataStore.cs:30: // TODO(peleyal): Is this the
interface that we want? or we want to have a factory like the JAVA library?
remove todo

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
Src/GoogleApis/Apis/Util/Store/IDataStore.cs:34: /// </summary>
one line

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
Src/GoogleApis/Apis/Util/Store/IDataStore.cs:44: ///  Returns the stored value
for the given key or <c>null</c> if not found.
one line

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
Src/GoogleApis/Apis/Util/Store/IDataStore.cs:50: /// </summary>
one line

[peleyal, 2013-09-10 12:32:21]

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Authenticati...
File Src/GoogleApis.Authentication.OAuth2/OAuth2Authenticator.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Authenticati...
Src/GoogleApis.Authentication.OAuth2/OAuth2Authenticator.cs:26: using
System.Threading.Tasks;
On 2013/09/10 01:14:44, peleyal wrote:
> Fix using order

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
File Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:27: /// File data store
that implements <see cref="IDataStore"/>. This store creates a different file
for each
On 2013/09/10 01:14:44, peleyal wrote:
> <seealso?

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:37: /// Constructs a new
file data store with the specified folder. This folder is going to be created
(if it's not
On 2013/09/10 01:14:44, peleyal wrote:
> is created 

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:51: /// Stores the given
value for the given key. It creates a new file (named <see
cref="GetStoredKey"/>) in
On 2013/09/10 01:14:44, peleyal wrote:
> change GetStoredKey to be static public

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.DotNet4/Apis...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:124: ///
<returns></returns>
On 2013/09/10 01:14:44, peleyal wrote:
> remove returns statement

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
File Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:254: ///
<summary> A message handler which returns a HTTP response message or throw an
exception. </summary>
On 2013/09/10 01:14:44, peleyal wrote:
> remove space at the end of the comment

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
File Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterCollectionTest.cs
(right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterCollectionTest.cs:23:
using Google.Apis.Requests.Parameters;
On 2013/09/10 01:14:44, peleyal wrote:
> reorder using

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
File Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterValidatorTest.cs
(right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/Apis/R...
Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterValidatorTest.cs:24:
using Google.Apis.Requests.Parameters;
On 2013/09/10 01:14:44, peleyal wrote:
> reorder usings

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/[Mock]...
File Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis.Tests/[Mock]...
Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs:23: using
System.Threading.Tasks;
On 2013/09/10 01:14:44, peleyal wrote:
> reorder usings

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Http/Ba...
File Src/GoogleApis/Apis/Http/BackOffHandler.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Http/Ba...
Src/GoogleApis/Apis/Http/BackOffHandler.cs:56: /// The  default is <see
cref="DefaultHandleExceptionFunc"/>.
On 2013/09/10 01:14:44, peleyal wrote:
> remove extra space
> The default (single space)

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Http/Ba...
Src/GoogleApis/Apis/Http/BackOffHandler.cs:136: await Handle(args.SupportsRetry,
args.CurrentFailedTry, args.CancellationToken);
On 2013/09/10 01:14:44, peleyal wrote:
> rename HandleAsync

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
File Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:27: public static
class ParameterUtils
On 2013/09/10 01:14:44, peleyal wrote:
> add comment

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:31: /// input request.
It uses reflection to look at all properties marked with
On 2013/09/10 01:14:44, peleyal wrote:
> to get all properties with the ... attribute

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:45: /// Creates a
parameter dictionary by using reflection to look at all properties marked with
On 2013/09/10 01:14:44, peleyal wrote:
> look --> get all 

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:59: /// Creates a
parameter dictionary by using reflection to look at all properties marked with
On 2013/09/10 01:14:44, peleyal wrote:
> ditto

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:70: private static
void IterateParameters(object request, Action<RequestParameterType, string,
object> action)
On 2013/09/10 01:14:44, peleyal wrote:
> add comment

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
File Src/GoogleApis/Apis/Requests/RequestBuilder.cs (left):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Request...
Src/GoogleApis/Apis/Requests/RequestBuilder.cs:28: /// <summary> Utility class
used to build a request from the various parts of a REST Method call. </summary>
On 2013/09/10 01:14:44, peleyal wrote:
> explain further.

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
File Src/GoogleApis/Apis/Util/Store/IDataStore.cs (right):

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
Src/GoogleApis/Apis/Util/Store/IDataStore.cs:30: // TODO(peleyal): Is this the
interface that we want? or we want to have a factory like the JAVA library?
On 2013/09/10 01:14:44, peleyal wrote:
> remove todo

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
Src/GoogleApis/Apis/Util/Store/IDataStore.cs:34: /// </summary>
On 2013/09/10 01:14:44, peleyal wrote:
> one line

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
Src/GoogleApis/Apis/Util/Store/IDataStore.cs:44: ///  Returns the stored value
for the given key or <c>null</c> if not found.
On 2013/09/10 01:14:44, peleyal wrote:
> one line

Done.

https://codereview.appspot.com/13412046/diff/3001/Src/GoogleApis/Apis/Util/St...
Src/GoogleApis/Apis/Util/Store/IDataStore.cs:50: /// </summary>
On 2013/09/10 01:14:44, peleyal wrote:
> one line

Done.

[peleyal, 2013-09-10 12:37:02]

Hi Mr. Class

I started splitting the huge CL into small huge CLs :)
Let me know if it looks too huge, so I'll split it further, I didn't do that
yet, because although there are a lot of files in this CL, it's actually a small
small change in most of them (Like updating to Bcl.Build 1.10 or rename 'http to
'HTTP')

Let me know,
Thanks!
Eyal

[class, 2013-09-16 23:47:29]

Lots of nits, mostly optional feedback.  I had a few comments on spacing,
particularly around comment tags, I usually use <tag>Content.</tag> like Visual
Studio autocompletes and you are mostly using <tag> Content. </tag> which looks
a little odd to me.

I'm slightly concerned around needing to manually update the Bcl.build versions
when NuGet updates, let me know if this will not be a problem either by NuGet
always getting the same version or if there is some magic that automatically
updates the versions and paths with Bcl.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Authenticat...
File Src/GoogleApis.Authentication.OAuth2.Tests/packages.config (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Authenticat...
Src/GoogleApis.Authentication.OAuth2.Tests/packages.config:5: <package
id="Microsoft.Bcl.Build" version="1.0.10" targetFramework="net40" />
I'm just wondering, if we give a specific version for these, will NuGet get that
version? If not, there could potentially be an issue when versions update,
particularly if we are setting the build targets to a specific version.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Authenticat...
File
Src/GoogleApis.Authentication.OAuth2/GoogleApis.Authentication.OAuth2.csproj
(right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Authenticat...
Src/GoogleApis.Authentication.OAuth2/GoogleApis.Authentication.OAuth2.csproj:102:
<Import
Project="..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets"
Condition="Exists('..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets')"
/>
Again, this is the specific version, I'm concerned that NuGet will auto-update
to more recent versions and cause these settings to be wrong, require the
developer to "hack" this value to match whatever NuGet updates to.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
File Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:37: /// Constructs a new
file data store with the specified folder. This folder is created (if it's not
exist yet)
if it's not exist yet - rephrase as "if it doesn't exist yet"

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:53: /// </summary>
Please add parameter tags here.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:69: /// </summary>
Please add parameters, these are probably the same as Store

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:91: ///  in <see
cref="FolderPath"/> doesn't exists.
Singular / plural on the summary:
   in <see cref="FolderPath"/> doesn't exists.

Should be:
    in <see cref="FolderPath"/> doesn't **exist**.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:93: public Task<T>
Get<T>(string key)
key needs a <param> tag

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:137: /// <param
name="t">The type to store or retrieve</param>
What is the "type" here, is there an enum somewhere for this?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Goo...
File Src/GoogleApis.DotNet4/GoogleApis.DotNet4.csproj (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Goo...
Src/GoogleApis.DotNet4/GoogleApis.DotNet4.csproj:95: <Import
Project="..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets"
Condition="Exists('..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets')"
/>
This is specific to a version, will NuGet respect the version or does the
developer need to "hack" this every time a new BCL build version comes out?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Pro...
File Src/GoogleApis.DotNet4/Properties/AssemblyInfo.cs (left):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Pro...
Src/GoogleApis.DotNet4/Properties/AssemblyInfo.cs:26: [assembly:
AssemblyCompany("Google Inc")]
Should you have an AssemblyVersion / AssemblyFileVersion here?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Http/ConfigurableMessageHandlerTest.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/ConfigurableMessageHandlerTest.cs:45: public
Task<bool> HandleResponseAsync(HandleUnsuccessfulResponseArgs args)
Missing documentation tags, not sure how important in the tests but might be
useful to at least say what is being tested.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:404: // we expect
a task canceled exception in case the canceled request is less or equal total
Typo, s/canceled/cancelled/

Also, consider capitalizing and adding period to comment and making this more
concise:

If the task was cancelled, the request number should not exceed the number of
retries.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:411:
Assert.True(cancelRequestNum > service.HttpClient.MessageHandler.NumTries);
Should this be assert.False (cancelRequestNum > ... As I understand it, this
should be less than or equal to the number of retries.

If you are testing something else, please add it to the comment.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:421: public void
Execute_DisposeService()
Missing test summary, consider adding.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:441: private void
SubtestExecute_GZip(bool gzip, bool async)
Consider adding parameters.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:444: {
Why is this brace indented twice, is it because it's a continuation? Consider:

    var handler = new TestBodyMessageHandler()
    {
        ...
    }

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:472: // NOTICE:
even if GZipEnabled is true, we don't need to extract the real string from the
GZip stream,
Replace Notice with Note and capitalize:

Note: Even if GZipEnabled is true, we don't...

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:477: // the
returned response should contain ETag, check that the service add the right ETag
property on
Capitalize The and add a period.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:484: ///
<summary> Tests execute when GZip is enabled. </summary>
(Optional)
It might just be me, but the spaces between the tags look a little funny, I
usually markup like:


<summary>Tests execute when</summary>

This is how the tags appear in the examples on MSDN

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:516: var handler
= new TestBodyMessageHnalder()
TestBodyMessageHnalder is misspelled, n and a are switched, should be
TestBodyMessageHanlder

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:542: /// A
subtest for testing execute when an exception is thrown during sending the
request, with or without
Consider rephrasing the summary:

A subtest for testing when an exception is thrown while sending the request.
This is tested with and without back-off. If the back-off handler is attached to
the service's message handler, there should be 3 tries (the default) before the
operation fails. This test is performed synchronously.

(Not sure about the last part).

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:558:
ExponentialBackOffPolicy.Exception :
It's a little weird to see the cases on separate lines, might be clearer to
place them on the same line if it fits within your column style.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:590: /// A
subtest for testing async execute when an exception is thrown during sending the
request, with or without
Please update the description to match the sync description, for example:

An asynchronous subtest for testing when an exception is thrown while sending
the request. This is tested with and without back-off. If the back-off handler
is attached to the service's message handler, there should be 3 tries (the
default) before the operation fails.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:606:
ExponentialBackOffPolicy.Exception :
Same here, consider putting these as a one-liner.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:760: var
initializer = new BaseClientService.Initializer
Spacing, consider adding a newline between each declaration.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:942: {
If you are using the indented spacing on declaration continuations (as done
above) this should be indented one more time.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:953: public
override string RestPath
Consider adding a <summary> tag

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:1007: using (var
service = new MockClientService(initializer, "https://build_request_params"))
Consider adding a line above.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterCollectionTest.cs
(right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterCollectionTest.cs:27: ///
<summary>Tests for the ParameterCollection class.</summary>
Looks like you are not adding spaces here (as in ClientServiceRequestTests.cs),
I prefer this style of tags in comments.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterValidatorTest.cs
(right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterValidatorTest.cs:25: ///
<summary> Tests <see cref="Google.Api.Requests.ParameterValidator"/>. </summary>
Weird whitespacing inside the <summary> tag.

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

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:61: private void
CheckDeserializationResults(MockJsonSchema result)
Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:69: private
IClientService CreateClientServiceV_03()
Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:87: /// <summary>
This tests the v0.3 deserialization of the BaseService. </summary>
What is v0.3?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:148: /// <summary>
Tests if serialization works. </summary>
Weird spacing on <summary> tags

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:182: /// <summary>
Tests the deserialization for server error responses. </summary>
rephrase: Tests deserialization for server error responses.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:219: /// Mock
authentication message handler which returns unauthorized response in the first
call, and in the
rephrase <summary>

A mock authentication message handler which returns an unauthorized response in
the first call and a successful response in the second.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:257: public int
ApplyCalls { get; set; }
<summary> missing

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:259: public void
ApplyAuthenticationToRequest(System.Net.HttpWebRequest request)
<summary> and <param> missing

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:276: /// <summary>
Mock Authenticator which handles unsuccessful response by "refreshing" the
token. </summary>
Missing <param> for args

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:279: public int
HandleCalls { get; set; }
Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:281: public
Task<bool> HandleResponseAsync(HandleUnsuccessfulResponseArgs args)
missing <summary>, I take it this is just implementing an interface, so
(optional)

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:292: /// Tests that
authenticator helpers invokes both apply authentication and handle response
methods on the
Revise: Tests that the authenticator helpers, when invoked, both apply
authentication and handle response methods on the authentication instance.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:318: /// Tests an
authenticator which doesn't implement unsuccessful response handler, and as a
result the request
revise:

Tests an authenticator which doesn't implement **an** unsuccessful response
handler. The request should fail when the token is invalid.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:346: public void
Constructor_DefaultValues()
Consider a <summary> with any relevant notes on what you're testing in the
constructor.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Utils/ExponentialBackOffTest.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Utils/ExponentialBackOffTest.cs:52: /// <summary>
Tests constructor with invalid time span object (less then 0 or greater than
1sec). </summary>
Spaces on <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Googl...
File Src/GoogleApis.Tests/GoogleApis.Tests.csproj (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Googl...
Src/GoogleApis.Tests/GoogleApis.Tests.csproj:185: <Import
Project="..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets"
Condition="Exists('..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets')"
/>
Same as comment in other files.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/[Mock...
File Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/[Mock...
Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs:47: protected override Task
Wait(TimeSpan ts, CancellationToken cancellationToken)
(optional) Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Authen...
File Src/GoogleApis/Apis/Authentication/AuthenticatorHelpers.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Authen...
Src/GoogleApis/Apis/Authentication/AuthenticatorHelpers.cs:44: public Task
InterceptAsync(HttpRequestMessage request, CancellationToken cancellationToken)
Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/B...
File Src/GoogleApis/Apis/Http/BackOffHandler.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/B...
Src/GoogleApis/Apis/Http/BackOffHandler.cs:104: /// <summary> Constructs a new
back-off handler with the given back-off. </summary>
Missing <param>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/E...
File Src/GoogleApis/Apis/Http/ExponentialBackOffInitializer.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/E...
Src/GoogleApis/Apis/Http/ExponentialBackOffInitializer.cs:25: /// Indicates if
exponential back-off is used automatically on exception in a service request
and\or when 503
Consider:

Indicates if exponential back-off is used automatically on **exceptions** in a
service **requests** and\or when 503

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/E...
Src/GoogleApis/Apis/Http/ExponentialBackOffInitializer.cs:42: private
ExponentialBackOffPolicy Policy { get; set; }
Missing <summary>

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

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:43: /// <summary> Sets an empty HTTP
content. </summary>
Spacing on <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/I...
File Src/GoogleApis/Apis/Http/IHttpExecuteInterceptor.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/I...
Src/GoogleApis/Apis/Http/IHttpExecuteInterceptor.cs:29: /// <summary> Invoked
before the request is being sent. </summary>
<param> tags missing

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:35: /// <seealso
cref="Google.Apis.Util.RequestParameterAttribute"/> attribute.
<param> missing

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:47: /// <summary>
<param missing>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:62: /// Creates a
parameter dictionary by using reflection to iterate over all properties with
<param> missing

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:74: /// Iterates over
all <seealso cref="RequestParameterAttribute"/> properties in the request object
and invoke
Iterates over all <seealso cref="RequestParameterAttribute"/> properties in the
request object and **invokes**

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:78: /// <param
name="action">An action to invoke which gets the parameter type, name and it
value</param>
Fix plural and add period at end of action param

<param name="action">An action to invoke which gets the parameter type, name and
°°its** value.</param>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:81: // use reflection
to build the parameter dictionary.
Capitalize use

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:85: // retrieve the
RequestParameterAttribute
Capitalize Retrieve and add . at end of comment

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:94: // get the name of
this parameter from the attribute, if it doesn't exist take a lower-case variant
of
Add period and capitol to comment

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:101: // call action
with the type name and value
Add period / cap to comment

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

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:132: // create a HTTP client
for this service
Add capital and period

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:144: // if factory wasn't set
use the default HTTP client factory
Cap / period

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

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/IClientService.cs:38: /// <summary> Gets the HTTP
client which is used to create requests. </summary>
Odd spaces on markup, consider <summary>Gets ...

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/I...
File Src/GoogleApis/Apis/Util/IClock.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/I...
Src/GoogleApis/Apis/Util/IClock.cs:32: public class SystemClock : IClock
It probably doesn't make sense for this release, but consider using Jon Skeet's
NodaTime - http://nodatime.org/

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/R...
File Src/GoogleApis/Apis/Util/RequestParameterAttribute.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/R...
Src/GoogleApis/Apis/Util/RequestParameterAttribute.cs:41: /// </summary>
Misssing <param>name

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/R...
Src/GoogleApis/Apis/Util/RequestParameterAttribute.cs:49: /// <param
name="name">
Is this supposed to be up on the previous line?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/R...
Src/GoogleApis/Apis/Util/RequestParameterAttribute.cs:66: /// The parameter is a
path parameter and will be inserted into the path portion of the request URI.
Consider removing "The parameter" .... for conciseness.

A path inserted into the path portion of the request URI.  


Same below

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/[Media...
File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:228: public Task<bool>
HandleResponseAsync(HandleUnsuccessfulResponseArgs args)
Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:248: public Task<bool>
HandleExceptionAsync(HandleExceptionArgs args)
Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/GoogleApis....
File Src/GoogleApis/GoogleApis.csproj (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/GoogleApis....
Src/GoogleApis/GoogleApis.csproj:44: <None
Include="Apis\[Media]\ClassDiagram.cd" />
Is it common to include the Class diagrams in these releases? They can be
generated by the developer if needed.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Properties/...
File Src/GoogleApis/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Properties/...
Src/GoogleApis/Properties/AssemblyInfo.cs:27: [assembly: AssemblyProduct("")]
Missing AssemblyVersion, AssemblyVersionName

https://codereview.appspot.com/13412046/diff/47001/packages/Microsoft.Bcl.Bui...
File packages/Microsoft.Bcl.Build.1.0.8/tools/Microsoft.Bcl.Build.targets
(left):

https://codereview.appspot.com/13412046/diff/47001/packages/Microsoft.Bcl.Bui...
packages/Microsoft.Bcl.Build.1.0.8/tools/Microsoft.Bcl.Build.targets:3:
Microsoft.Bcl.targets
Is it necessary to ship with Bcl targets? Just curious because I believe this is
correctly generated when you install the NuGet packages.

[peleyal, 2013-09-17 15:53:39]

WOWWWW, that took me forever.

Thanks Gus!

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Authenticat...
File Src/GoogleApis.Authentication.OAuth2.Tests/packages.config (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Authenticat...
Src/GoogleApis.Authentication.OAuth2.Tests/packages.config:5: <package
id="Microsoft.Bcl.Build" version="1.0.10" targetFramework="net40" />
I don't understand your exact point but:

1. You must specify a specific version.
2. You can use any version above 1.0.10 (I'm still not sure if it's only for
minor version or also for major version).
3. I don't think we have any other alternative for that.

The good point is that if we released a client library which uses 1.0.10, and a
day after it there is 1.0.11 out there, our clients will get 1.0.11 when they
install a new Google.Apis package. If they already installed a version, they
will need to run Update-Package with the package name (Microsoft.Bcl.Build) and
then they will get the new version.

Makes sense?


On 2013/09/16 23:47:29, class wrote:
> I'm just wondering, if we give a specific version for these, will NuGet get
that
> version? If not, there could potentially be an issue when versions update,
> particularly if we are setting the build targets to a specific version.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Authenticat...
File
Src/GoogleApis.Authentication.OAuth2/GoogleApis.Authentication.OAuth2.csproj
(right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Authenticat...
Src/GoogleApis.Authentication.OAuth2/GoogleApis.Authentication.OAuth2.csproj:102:
<Import
Project="..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets"
Condition="Exists('..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets')"
/>
The user doesn't need to do anything.
The csproj must have a reference to specific version.

Microsoft updates their bcl.build package, so if you don't have that file
(Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets) in your source
control, the project won't compile in the first time, but in the second time
everything will work.

I don't see any other option here as well

On 2013/09/16 23:47:29, class wrote:
> Again, this is the specific version, I'm concerned that NuGet will auto-update
> to more recent versions and cause these settings to be wrong, require the
> developer to "hack" this value to match whatever NuGet updates to.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
File Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:37: /// Constructs a new
file data store with the specified folder. This folder is created (if it's not
exist yet)
On 2013/09/16 23:47:29, class wrote:
> if it's not exist yet - rephrase as "if it doesn't exist yet"

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:53: /// </summary>
On 2013/09/16 23:47:29, class wrote:
> Please add parameter tags here.

Done. I'm not sure that I really added important information here, that's why I
didn't think to add it in the first place.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:69: /// </summary>
On 2013/09/16 23:47:29, class wrote:
> Please add parameters, these are probably the same as Store 

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:91: ///  in <see
cref="FolderPath"/> doesn't exists.
On 2013/09/16 23:47:29, class wrote:
> Singular / plural on the summary:
>    in <see cref="FolderPath"/> doesn't exists.
> 
> Should be:
>     in <see cref="FolderPath"/> doesn't **exist**.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:93: public Task<T>
Get<T>(string key)
On 2013/09/16 23:47:29, class wrote:
> key needs a <param> tag

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Api...
Src/GoogleApis.DotNet4/Apis/Util/Store/FileDataStore.cs:137: /// <param
name="t">The type to store or retrieve</param>
Type is the type of the class
(http://msdn.microsoft.com/en-us/library/system.type.aspx)
I explained in IDataStore that the key that we are going to store is combination
of key and type. In that case I can store both ints and string (for example)
with key = 4.
because the data store will store 2 different keys (int4 and string4).


On 2013/09/16 23:47:29, class wrote:
> What is the "type" here, is there an enum somewhere for this?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Goo...
File Src/GoogleApis.DotNet4/GoogleApis.DotNet4.csproj (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Goo...
Src/GoogleApis.DotNet4/GoogleApis.DotNet4.csproj:95: <Import
Project="..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets"
Condition="Exists('..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets')"
/>
The user doesn't need to do anything.
Let's talk about it in our next meeting... :)

On 2013/09/16 23:47:29, class wrote:
> This is specific to a version, will NuGet respect the version or does the
> developer need to "hack" this every time a new BCL build version comes out?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Pro...
File Src/GoogleApis.DotNet4/Properties/AssemblyInfo.cs (left):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.DotNet4/Pro...
Src/GoogleApis.DotNet4/Properties/AssemblyInfo.cs:26: [assembly:
AssemblyCompany("Google Inc")]
we have look down :)

[assembly: AssemblyVersion("1.5.0.*")]

On 2013/09/16 23:47:29, class wrote:
> Should you have an AssemblyVersion / AssemblyFileVersion here?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Http/ConfigurableMessageHandlerTest.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/ConfigurableMessageHandlerTest.cs:45: public
Task<bool> HandleResponseAsync(HandleUnsuccessfulResponseArgs args)
I don't like to add documentation of inherited methods, cause the documentation
should be in the interface. I don't like to copy it, because if something will
change in the interface, we will need to update it here too.

On 2013/09/16 23:47:29, class wrote:
> Missing documentation tags, not sure how important in the tests but might be
> useful to at least say what is being tested.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:404: // we expect
a task canceled exception in case the canceled request is less or equal total
LOL http://grammarist.com/spelling/cancel/
I think that because TaskCanceledException is with one 'l' I'll stay with the
same convention

I'll start follow your suggestion for new code. But not for exists one
On 2013/09/16 23:47:29, class wrote:
> Typo, s/canceled/cancelled/
> 
> Also, consider capitalizing and adding period to comment and making this more
> concise:
> 
> If the task was cancelled, the request number should not exceed the number of
> retries.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:411:
Assert.True(cancelRequestNum > service.HttpClient.MessageHandler.NumTries);
On 2013/09/16 23:47:29, class wrote:
> Should this be assert.False (cancelRequestNum > ... As I understand it, this
> should be less than or equal to the number of retries.
> 
> If you are testing something else, please add it to the comment.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:421: public void
Execute_DisposeService()
On 2013/09/16 23:47:29, class wrote:
> Missing test summary, consider adding.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:441: private void
SubtestExecute_GZip(bool gzip, bool async)
On 2013/09/16 23:47:29, class wrote:
> Consider adding parameters.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:444: {
On 2013/09/16 23:47:29, class wrote:
> Why is this brace indented twice, is it because it's a continuation? Consider:
> 
>     var handler = new TestBodyMessageHandler()
>     {
>         ...
>     }

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:472: // NOTICE:
even if GZipEnabled is true, we don't need to extract the real string from the
GZip stream,
On 2013/09/16 23:47:29, class wrote:
> Replace Notice with Note and capitalize:
> 
> Note: Even if GZipEnabled is true, we don't...

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:477: // the
returned response should contain ETag, check that the service add the right ETag
property on
On 2013/09/16 23:47:29, class wrote:
> Capitalize The and add a period.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:484: ///
<summary> Tests execute when GZip is enabled. </summary>
On 2013/09/16 23:47:29, class wrote:
> (Optional)
> It might just be me, but the spaces between the tags look a little funny, I
> usually markup like:
> 
> 
> <summary>Tests execute when</summary>
> 
> This is how the tags appear in the examples on MSDN

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:516: var handler
= new TestBodyMessageHnalder()
On 2013/09/16 23:47:29, class wrote:
> TestBodyMessageHnalder is misspelled, n and a are switched, should be
> TestBodyMessageHanlder

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:542: /// A
subtest for testing execute when an exception is thrown during sending the
request, with or without
On 2013/09/16 23:47:29, class wrote:
> Consider rephrasing the summary:
> 
> A subtest for testing when an exception is thrown while sending the request.
> This is tested with and without back-off. If the back-off handler is attached
to
> the service's message handler, there should be 3 tries (the default) before
the
> operation fails. This test is performed synchronously.
> 
> (Not sure about the last part).

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:558:
ExponentialBackOffPolicy.Exception :
On 2013/09/16 23:47:29, class wrote:
> It's a little weird to see the cases on separate lines, might be clearer to
> place them on the same line if it fits within your column style.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:590: /// A
subtest for testing async execute when an exception is thrown during sending the
request, with or without
On 2013/09/16 23:47:29, class wrote:
> Please update the description to match the sync description, for example:
> 
> An asynchronous subtest for testing when an exception is thrown while sending
> the request. This is tested with and without back-off. If the back-off handler
> is attached to the service's message handler, there should be 3 tries (the
> default) before the operation fails.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:606:
ExponentialBackOffPolicy.Exception :
On 2013/09/16 23:47:29, class wrote:
> Same here, consider putting these as a one-liner.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:760: var
initializer = new BaseClientService.Initializer
On 2013/09/16 23:47:29, class wrote:
> Spacing, consider adding a newline between each declaration.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:942: {
On 2013/09/16 23:47:29, class wrote:
> If you are using the indented spacing on declaration continuations (as done
> above) this should be indented one more time.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:953: public
override string RestPath
Again it is a property that we inhrit, I don't like copy-paste comments from the
base class
On 2013/09/16 23:47:29, class wrote:
> Consider adding a <summary> tag

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs:1007: using (var
service = new MockClientService(initializer, "https://build_request_params"))
On 2013/09/16 23:47:29, class wrote:
> Consider adding a line above.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterCollectionTest.cs
(right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterCollectionTest.cs:27: ///
<summary>Tests for the ParameterCollection class.</summary>
I noticed that somewhere after I got the library.
New code creates the one that you suggested (without spaces), I'll try to
"convert" the old way to the new one, during the way :)
On 2013/09/16 23:47:29, class wrote:
> Looks like you are not adding spaces here (as in
ClientServiceRequestTests.cs),
> I prefer this style of tags in comments.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterValidatorTest.cs
(right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Requests/Parameters/ParameterValidatorTest.cs:25: ///
<summary> Tests <see cref="Google.Api.Requests.ParameterValidator"/>. </summary>
On 2013/09/16 23:47:29, class wrote:
> Weird whitespacing inside the <summary> tag.

Done.

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

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:61: private void
CheckDeserializationResults(MockJsonSchema result)
On 2013/09/16 23:47:29, class wrote:
> Missing <summary>

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:69: private
IClientService CreateClientServiceV_03()
On 2013/09/16 23:47:29, class wrote:
> Missing <summary>

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:87: /// <summary>
This tests the v0.3 deserialization of the BaseService. </summary>
An old version of discovery.
You know what, I'm going to get rid of that code, cause there is no support in
0.3 anymore.


On 2013/09/16 23:47:29, class wrote:
> What is v0.3?

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:148: /// <summary>
Tests if serialization works. </summary>
On 2013/09/16 23:47:29, class wrote:
> Weird spacing on <summary> tags

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:182: /// <summary>
Tests the deserialization for server error responses. </summary>
On 2013/09/16 23:47:29, class wrote:
> rephrase: Tests deserialization for server error responses.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:219: /// Mock
authentication message handler which returns unauthorized response in the first
call, and in the
On 2013/09/16 23:47:29, class wrote:
> rephrase <summary>
> 
> A mock authentication message handler which returns an unauthorized response
in
> the first call and a successful response in the second.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:257: public int
ApplyCalls { get; set; }
On 2013/09/16 23:47:29, class wrote:
> <summary> missing

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:259: public void
ApplyAuthenticationToRequest(System.Net.HttpWebRequest request)
On 2013/09/16 23:47:29, class wrote:
> <summary> and <param> missing

ditto (inherit from IAuthenticator)

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:276: /// <summary>
Mock Authenticator which handles unsuccessful response by "refreshing" the
token. </summary>
On 2013/09/16 23:47:29, class wrote:
> Missing <param> for args

??? not here

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:279: public int
HandleCalls { get; set; }
On 2013/09/16 23:47:29, class wrote:
> Missing <summary>

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:281: public
Task<bool> HandleResponseAsync(HandleUnsuccessfulResponseArgs args)
ditto
On 2013/09/16 23:47:29, class wrote:
> missing <summary>, I take it this is just implementing an interface, so
> (optional)

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:292: /// Tests that
authenticator helpers invokes both apply authentication and handle response
methods on the
On 2013/09/16 23:47:29, class wrote:
> Revise: Tests that the authenticator helpers, when invoked, both apply
> authentication and handle response methods on the authentication instance.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:318: /// Tests an
authenticator which doesn't implement unsuccessful response handler, and as a
result the request
On 2013/09/16 23:47:29, class wrote:
> revise:
> 
> Tests an authenticator which doesn't implement **an** unsuccessful response
> handler. The request should fail when the token is invalid.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:346: public void
Constructor_DefaultValues()
On 2013/09/16 23:47:29, class wrote:
> Consider a <summary> with any relevant notes on what you're testing in the
> constructor.

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Utils/ExponentialBackOffTest.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Utils/ExponentialBackOffTest.cs:52: /// <summary>
Tests constructor with invalid time span object (less then 0 or greater than
1sec). </summary>
On 2013/09/16 23:47:29, class wrote:
> Spaces on <summary>

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Googl...
File Src/GoogleApis.Tests/GoogleApis.Tests.csproj (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/Googl...
Src/GoogleApis.Tests/GoogleApis.Tests.csproj:185: <Import
Project="..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets"
Condition="Exists('..\..\packages\Microsoft.Bcl.Build.1.0.10\tools\Microsoft.Bcl.Build.targets')"
/>
ditto
On 2013/09/16 23:47:29, class wrote:
> Same as comment in other files.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/[Mock...
File Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis.Tests/[Mock...
Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs:47: protected override Task
Wait(TimeSpan ts, CancellationToken cancellationToken)
ditto
On 2013/09/16 23:47:29, class wrote:
> (optional) Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Authen...
File Src/GoogleApis/Apis/Authentication/AuthenticatorHelpers.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Authen...
Src/GoogleApis/Apis/Authentication/AuthenticatorHelpers.cs:44: public Task
InterceptAsync(HttpRequestMessage request, CancellationToken cancellationToken)
ditto (comment is in the interface level)
On 2013/09/16 23:47:29, class wrote:
> Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/B...
File Src/GoogleApis/Apis/Http/BackOffHandler.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/B...
Src/GoogleApis/Apis/Http/BackOffHandler.cs:104: /// <summary> Constructs a new
back-off handler with the given back-off. </summary>
On 2013/09/16 23:47:29, class wrote:
> Missing <param>

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/E...
File Src/GoogleApis/Apis/Http/ExponentialBackOffInitializer.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/E...
Src/GoogleApis/Apis/Http/ExponentialBackOffInitializer.cs:25: /// Indicates if
exponential back-off is used automatically on exception in a service request
and\or when 503
On 2013/09/16 23:47:29, class wrote:
> Consider:
> 
> Indicates if exponential back-off is used automatically on **exceptions** in a
> service **requests** and\or when 503 

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/E...
Src/GoogleApis/Apis/Http/ExponentialBackOffInitializer.cs:42: private
ExponentialBackOffPolicy Policy { get; set; }
On 2013/09/16 23:47:29, class wrote:
> Missing <summary>

Done.

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

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/H...
Src/GoogleApis/Apis/Http/HttpExtenstions.cs:43: /// <summary> Sets an empty HTTP
content. </summary>
On 2013/09/16 23:47:29, class wrote:
> Spacing on <summary>

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/I...
File Src/GoogleApis/Apis/Http/IHttpExecuteInterceptor.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Http/I...
Src/GoogleApis/Apis/Http/IHttpExecuteInterceptor.cs:29: /// <summary> Invoked
before the request is being sent. </summary>
On 2013/09/16 23:47:29, class wrote:
> <param> tags missing

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
File Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:35: /// <seealso
cref="Google.Apis.Util.RequestParameterAttribute"/> attribute.
On 2013/09/16 23:47:29, class wrote:
> <param> missing

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:47: /// <summary>
On 2013/09/16 23:47:29, class wrote:
> <param missing>

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:62: /// Creates a
parameter dictionary by using reflection to iterate over all properties with
On 2013/09/16 23:47:29, class wrote:
> <param> missing

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:74: /// Iterates over
all <seealso cref="RequestParameterAttribute"/> properties in the request object
and invoke
On 2013/09/16 23:47:29, class wrote:
> Iterates over all <seealso cref="RequestParameterAttribute"/> properties in
the
> request object and **invokes**

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:78: /// <param
name="action">An action to invoke which gets the parameter type, name and it
value</param>
On 2013/09/16 23:47:29, class wrote:
> Fix plural and add period at end of action param
> 
> <param name="action">An action to invoke which gets the parameter type, name
and
> °°its** value.</param>

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:81: // use reflection
to build the parameter dictionary.
On 2013/09/16 23:47:29, class wrote:
> Capitalize use

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:85: // retrieve the
RequestParameterAttribute
On 2013/09/16 23:47:29, class wrote:
> Capitalize Retrieve and add . at end of comment

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:94: // get the name of
this parameter from the attribute, if it doesn't exist take a lower-case variant
of
On 2013/09/16 23:47:29, class wrote:
> Add period and capitol to comment

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Reques...
Src/GoogleApis/Apis/Requests/Parameters/ParameterUtils.cs:101: // call action
with the type name and value
On 2013/09/16 23:47:29, class wrote:
> Add period / cap to comment

Done.

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

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:132: // create a HTTP client
for this service
On 2013/09/16 23:47:29, class wrote:
> Add capital and period

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:144: // if factory wasn't set
use the default HTTP client factory
On 2013/09/16 23:47:29, class wrote:
> Cap / period

Done.

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

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/IClientService.cs:38: /// <summary> Gets the HTTP
client which is used to create requests. </summary>
On 2013/09/16 23:47:29, class wrote:
> Odd spaces on markup, consider <summary>Gets ...

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/I...
File Src/GoogleApis/Apis/Util/IClock.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/I...
Src/GoogleApis/Apis/Util/IClock.cs:32: public class SystemClock : IClock
I'm already using a lot of packages. What is the big benefit of this package?
On 2013/09/16 23:47:29, class wrote:
> It probably doesn't make sense for this release, but consider using Jon
Skeet's
> NodaTime - http://nodatime.org/

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/R...
File Src/GoogleApis/Apis/Util/RequestParameterAttribute.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/R...
Src/GoogleApis/Apis/Util/RequestParameterAttribute.cs:41: /// </summary>
On 2013/09/16 23:47:29, class wrote:
> Misssing <param>name

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/R...
Src/GoogleApis/Apis/Util/RequestParameterAttribute.cs:49: /// <param
name="name">
On 2013/09/16 23:47:29, class wrote:
> Is this supposed to be up on the previous line?

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/Util/R...
Src/GoogleApis/Apis/Util/RequestParameterAttribute.cs:66: /// The parameter is a
path parameter and will be inserted into the path portion of the request URI.
On 2013/09/16 23:47:29, class wrote:
> Consider removing "The parameter" .... for conciseness.
> 
> A path inserted into the path portion of the request URI.  
> 
> 
> Same below 

Done.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/[Media...
File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:228: public Task<bool>
HandleResponseAsync(HandleUnsuccessfulResponseArgs args)
ditto
On 2013/09/16 23:47:29, class wrote:
> Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:248: public Task<bool>
HandleExceptionAsync(HandleExceptionArgs args)
ditto
On 2013/09/16 23:47:29, class wrote:
> Missing <summary>

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/GoogleApis....
File Src/GoogleApis/GoogleApis.csproj (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/GoogleApis....
Src/GoogleApis/GoogleApis.csproj:44: <None
Include="Apis\[Media]\ClassDiagram.cd" />
mmmmmm....
why not?
I can remove it it you want


On 2013/09/16 23:47:29, class wrote:
> Is it common to include the Class diagrams in these releases? They can be
> generated by the developer if needed.

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Properties/...
File Src/GoogleApis/Properties/AssemblyInfo.cs (right):

https://codereview.appspot.com/13412046/diff/47001/Src/GoogleApis/Properties/...
Src/GoogleApis/Properties/AssemblyInfo.cs:27: [assembly: AssemblyProduct("")]
No we have Assembly Version.
Why do we need AssemblyVersionName?
On 2013/09/16 23:47:29, class wrote:
> Missing AssemblyVersion, AssemblyVersionName

https://codereview.appspot.com/13412046/diff/47001/packages/Microsoft.Bcl.Bui...
File packages/Microsoft.Bcl.Build.1.0.8/tools/Microsoft.Bcl.Build.targets
(left):

https://codereview.appspot.com/13412046/diff/47001/packages/Microsoft.Bcl.Bui...
packages/Microsoft.Bcl.Build.1.0.8/tools/Microsoft.Bcl.Build.targets:3:
Microsoft.Bcl.targets
It was removed
On 2013/09/16 23:47:29, class wrote:
> Is it necessary to ship with Bcl targets? Just curious because I believe this
is
> correctly generated when you install the NuGet packages.

[class, 2013-09-21 03:12:10]

LGTM.

I found one small typo, and optionally, you can update the copyright date (some
files are 2010).

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

https://codereview.appspot.com/13412046/diff/92001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:36: /// It also contains
important properties like number of tires, follow redirect, etc.
> number of tires

Should this be number of retries?

https://codereview.appspot.com/13412046/diff/92001/Src/GoogleApis/GoogleApiEx...
File Src/GoogleApis/GoogleApiException.cs (right):

https://codereview.appspot.com/13412046/diff/92001/Src/GoogleApis/GoogleApiEx...
Src/GoogleApis/GoogleApiException.cs:2: Copyright 2010 Google Inc
(Optional) Update copyright date?

[peleyal, 2013-09-23 12:01:15]

Wax pushed to repository :)
Thanks Gus!

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

https://codereview.appspot.com/13412046/diff/92001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:36: /// It also contains
important properties like number of tires, follow redirect, etc.
tries

The property is NumTries.

On 2013/09/21 03:12:10, class wrote:
> > number of tires
> 
> Should this be number of retries?

https://codereview.appspot.com/13412046/diff/92001/Src/GoogleApis/GoogleApiEx...
File Src/GoogleApis/GoogleApiException.cs (right):

https://codereview.appspot.com/13412046/diff/92001/Src/GoogleApis/GoogleApiEx...
Src/GoogleApis/GoogleApiException.cs:2: Copyright 2010 Google Inc
As I understood from David (davidwaters) who owned the library before me, we
never update the copyright of old files, only in case we create a new file - we
use the current year.


On 2013/09/21 03:12:10, class wrote:
> (Optional) Update copyright date?

[peleyal, 2013-09-23 12:01:26]

Was pushed to repository :)
Thanks Gus!