|
|
DescriptionReimplement 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)
Patch Set 1 #Patch Set 2 : all tests are running #
Total comments: 42
Patch Set 3 : self review #Patch Set 4 : minor #
Total comments: 148
Patch Set 5 : Class comments #Patch Set 6 : minor #
Total comments: 4
MessagesTotal messages: 8
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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?
Sign in to reply to this message.
|