|
|
Created:
10 years, 10 months ago by peleyal Modified:
10 years, 9 months ago Reviewers:
David waters CC:
google-api-dotnet-client_googlegroups.com Base URL:
https://google-api-dotnet-client.googlecode.com/hg/ Visibility:
Public. |
DescriptionIssue 377: New build for releasing a new version
In this CL - I removed all tools projects and created two new ones - Google.Apis.Release and Google.Apis.Utils
Patch Set 1 #Patch Set 2 : Running step 2. Next commiting and pushing to all repositories - The real test! #Patch Set 3 : Using this tool to release 1.5.0-beta #Patch Set 4 : minor #Patch Set 5 : Hg pull -u #Patch Set 6 : add comments #
Total comments: 17
Patch Set 7 : self review #
Total comments: 1
Patch Set 8 : minor #
Total comments: 38
Patch Set 9 : self review II #Patch Set 10 : minor #
Total comments: 32
Patch Set 11 : david comments #MessagesTotal messages: 11
https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Program.cs (right): https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:155: } TODO(peleyal): release notes should be part of the package https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:167: Console.WriteLine("You upgrade the libraries in the samples repository and pushed that change."); update each sample to use the new NuGet packages (from local repository) https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:185: if (!HasIncomingChanges(AllRepositories)) Add comment here and also some printing in case there are incoming changes https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:196: Console.WriteLine("========================="); Make utility for the Press 'y' or 'yes' .... section https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:230: Add: now you should run the NuGet publisher to create a new PCL for each generated API https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:253: /// <summary>Displays the user orders how to create a branch.</summary> new branch to this release (only if this is a new minor \ major version https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:280: if (repository.Equals(DefaultRepository) && committed) Don't like if inside foreach think to move to different place maybe TagDefaultRepository method? https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:299: /// <summary>Updates the "Downloads" wiki page with the new release.</summary> the new release notes
Sign in to reply to this message.
https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Program.cs (right): https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:155: } On 2013/09/01 22:31:03, peleyal wrote: > TODO(peleyal): release notes should be part of the package Done. https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:167: Console.WriteLine("You upgrade the libraries in the samples repository and pushed that change."); On 2013/09/01 22:31:03, peleyal wrote: > update each sample to use the new NuGet packages (from local repository) Done. https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:185: if (!HasIncomingChanges(AllRepositories)) On 2013/09/01 22:31:03, peleyal wrote: > Add comment here and also some printing in case there are incoming changes Done. https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:196: Console.WriteLine("========================="); On 2013/09/01 22:31:03, peleyal wrote: > Make utility for the Press 'y' or 'yes' .... section Done. https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:230: On 2013/09/01 22:31:03, peleyal wrote: > Add: now you should run the NuGet publisher to create a new PCL for each > generated API Done. https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:253: /// <summary>Displays the user orders how to create a branch.</summary> On 2013/09/01 22:31:03, peleyal wrote: > new branch to this release (only if this is a new minor \ major version Done. https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:253: /// <summary>Displays the user orders how to create a branch.</summary> On 2013/09/01 22:31:03, peleyal wrote: > new branch to this release (only if this is a new minor \ major version Done. https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:280: if (repository.Equals(DefaultRepository) && committed) On 2013/09/01 22:31:03, peleyal wrote: > Don't like if inside foreach think to move to different place > maybe TagDefaultRepository method? Done. https://codereview.appspot.com/12767046/diff/14001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:299: /// <summary>Updates the "Downloads" wiki page with the new release.</summary> On 2013/09/01 22:31:03, peleyal wrote: > the new release notes Done.
Sign in to reply to this message.
https://codereview.appspot.com/12767046/diff/21001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Resources/License.txt (right): https://codereview.appspot.com/12767046/diff/21001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Resources/License.txt:190: Copyright 2013 Google Inc. Copyright [yyyy] [name of copyright owner]
Sign in to reply to this message.
https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Program.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:1: using System; add Google header https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:36: "'3' for publishing on the local packages to NuGet"; remove 3 https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:37: const string IsBetaHelpText = "Is this release is beta?"; Is this release beta? https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:49: HelpText = VersionHelpText)] one line https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:67: /// <summary> Command line arguments. </summary> remove space before and after https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:113: var p = new Program(options) s/p/program https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:177: // if there incoming changes those changes will be printed, otherwsie we can continue in the if there are https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:211: Console.WriteLine("Now... you should run the NuGet publisher to publish a new PCL " NuGetPublisher.exe --all_apis true -m publisher -k [NUGET_KEY_HERE] https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:268: // TODO(peleyal): automate this as well consider automate https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:319: /// <returns></returns> <returns>The release notes of this version https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:337: why is there empty line here? same below https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:546: /// Builds "default" repository projects one line. <return><c>true</c> if the default repository was build successfully https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:588: public Program(Options options) comment https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/ProjectExtenstions.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/ProjectExtenstions.cs:1: using System; Google header https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Properties/AssemblyInfo.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Properties/AssemblyInfo.cs:1: using System.Reflection; copyright https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Repositories/Hg.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Repositories/Hg.cs:93: /// </summary> remove </summary> https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Repositories/Hg.cs:192: /// <summary>Runs a HG command. In addition it prints error and message to trace.</summary> errors and messages https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs:1: using System; header https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Utils/Pr... File Tools/Google.Apis.Utils/Properties/AssemblyInfo.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Utils/Pr... Tools/Google.Apis.Utils/Properties/AssemblyInfo.cs:1: using System.Reflection; header
Sign in to reply to this message.
https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Program.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:1: using System; On 2013/09/14 21:15:14, peleyal wrote: > add Google header Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:36: "'3' for publishing on the local packages to NuGet"; On 2013/09/14 21:15:14, peleyal wrote: > remove 3 Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:37: const string IsBetaHelpText = "Is this release is beta?"; On 2013/09/14 21:15:14, peleyal wrote: > Is this release beta? Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:49: HelpText = VersionHelpText)] On 2013/09/14 21:15:14, peleyal wrote: > one line Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:67: /// <summary> Command line arguments. </summary> On 2013/09/14 21:15:14, peleyal wrote: > remove space before and after Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:113: var p = new Program(options) On 2013/09/14 21:15:14, peleyal wrote: > s/p/program Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:177: // if there incoming changes those changes will be printed, otherwsie we can continue in the On 2013/09/14 21:15:14, peleyal wrote: > if there are Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:211: Console.WriteLine("Now... you should run the NuGet publisher to publish a new PCL " On 2013/09/14 21:15:14, peleyal wrote: > NuGetPublisher.exe --all_apis true -m publisher -k [NUGET_KEY_HERE] Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:268: // TODO(peleyal): automate this as well On 2013/09/14 21:15:14, peleyal wrote: > consider automate Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:319: /// <returns></returns> On 2013/09/14 21:15:14, peleyal wrote: > <returns>The release notes of this version Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:337: On 2013/09/14 21:15:14, peleyal wrote: > why is there empty line here? same below Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:546: /// Builds "default" repository projects On 2013/09/14 21:15:14, peleyal wrote: > one line. > <return><c>true</c> if the default repository was build successfully Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:588: public Program(Options options) On 2013/09/14 21:15:14, peleyal wrote: > comment Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/ProjectExtenstions.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/ProjectExtenstions.cs:1: using System; On 2013/09/14 21:15:14, peleyal wrote: > Google header Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Properties/AssemblyInfo.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Properties/AssemblyInfo.cs:1: using System.Reflection; On 2013/09/14 21:15:14, peleyal wrote: > copyright Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Repositories/Hg.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Repositories/Hg.cs:93: /// </summary> On 2013/09/14 21:15:14, peleyal wrote: > remove </summary> Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Repositories/Hg.cs:192: /// <summary>Runs a HG command. In addition it prints error and message to trace.</summary> On 2013/09/14 21:15:14, peleyal wrote: > errors and messages Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs:1: using System; On 2013/09/14 21:15:14, peleyal wrote: > header Done. https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Utils/Pr... File Tools/Google.Apis.Utils/Properties/AssemblyInfo.cs (right): https://codereview.appspot.com/12767046/diff/26001/Tools/Google.Apis.Utils/Pr... Tools/Google.Apis.Utils/Properties/AssemblyInfo.cs:1: using System.Reflection; On 2013/09/14 21:15:14, peleyal wrote: > header Done.
Sign in to reply to this message.
Hi David, I had 2 self reviews, and I feel comfortable with this one. I know that you don't have time for full review process, so give your best in this one, and I'll follow your comments, without expecting for second round. Thanks, Eyal
Sign in to reply to this message.
Hi Eyal, Pretty good code just a few thoughts. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Program.cs (right): https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:95: private Hg DefaultRepository { get; set; } If Hg has not changed since I last looked at it ( about a year ago ) it is quite rubish. You may wish to do a review of it before using it more. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:120: if(options.Step > 2 || < 1) user.tell("Dude! got {0} expecting 1 or 2 "); https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:157: if (options.Step == 1) Consider if(step 1) { doBaseStep(); // Or better name } else if(step 2) { doApiSteps(); // Or better name } else { user.tell("How the hell did you get here?"); } https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:164: if (!HasIncomingChanges(new List<Hg> { DefaultRepository })) Do vararg parameters in c# allow passing in lists? or just array? if lists you might want to if(!HasIncomingChagnes(DefaultRepo)) ... private bool HasIncomingChanges(Hg... repos) https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:168: CreateNuGetPackages(); CreateCoreNuGetPackages ? https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:170: } Good to have else clauses for both these if statments else { user.Tell("Failed to build default Pacakge"); } else { user.Tell("Cant build with outstanding changes"); } Please disregard if the methods already warn the user before returning false. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:186: if (CanContinue()) Consider:I would reverse this if condition to reduce nesting. if(!CanContinue()) { return; } Dito HasIncomingChange https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:188: SamplesRepository = new Hg(new Uri(string.Format(CloneUrl, ".samples")), "samples"); Is constructing new Hg expensive? If not it may be worth doing private void Run() { initAllRepos(); if (step 1) ... Currently currently DefaultRepository is null so HasIncomingChanges(AllRepositories) will not check for incoming changes in DefaultRepo. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:255: input = Console.ReadLine(); consider: input = Console.ReadLine().toLowerCase(); https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:347: #region [RELEASE_VERSION]/Generated/Bin Consider:I would be inclinded to break these #regions out into methods. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/ProjectExtenstions.cs (right): https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/ProjectExtenstions.cs:28: /// <summary>Project extensions.</summary> Better doc please. maybe just Extension methods for the Project class but if you could add a little detail about what sort of functionality you are added it would be good. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/ProjectExtenstions.cs:29: static class ProjectExtenstions can this class be internal? ( I never remember what the default vis in c# is.) https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs (right): https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs:22: /// <summary>Updates the Downloads wiki page. </summary> please give live url to page https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs:30: { consider : workingDirectory.ThrowIfNullOrEmpty() dito oldVersion & newVersion https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs:72: Func<string, string> replaceFunc) Consider, This feels like over-generalization we have twice as many overloads as calls! :) I would drop this overload and drop the func. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Utils/Tr... File Tools/Google.Apis.Utils/TraceExtensions.cs (right): https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Utils/Tr... Tools/Google.Apis.Utils/TraceExtensions.cs:27: /// <summary>Traces the event using the current thread id.</summary> Consider: I no longer like making extension methods public, cause I did early in this project and now regrate it. Either keep internal or maybe move into its own namespace so no-one starts using this method by accident (inteli-sense).
Sign in to reply to this message.
Thanks for the review! Let me know if don't have time for last review, so I'll just push it. I've not changed a lot, so I'm feeling comfortable to push it without your LGTM. Let me know, have a great wkd! Eyal https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Program.cs (right): https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:95: private Hg DefaultRepository { get; set; } I changed it a little bit. It does the job, and we will focus later on, on moving to github so we may remove this piece of code On 2013/09/19 16:22:32, David waters wrote: > If Hg has not changed since I last looked at it ( about a year ago ) it is quite > rubish. > You may wish to do a review of it before using it more. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:120: On 2013/09/19 16:22:32, David waters wrote: > if(options.Step > 2 || < 1) > user.tell("Dude! got {0} expecting 1 or 2 "); Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:157: if (options.Step == 1) On 2013/09/19 16:22:32, David waters wrote: > Consider > if(step 1) > { > doBaseStep(); // Or better name > } else if(step 2) { > doApiSteps(); // Or better name > } else { > user.tell("How the hell did you get here?"); > } Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:164: if (!HasIncomingChanges(new List<Hg> { DefaultRepository })) I hate vargas. I prefer enumerable. That's my style :) enumerable is very easy to use and it supports linq :) On 2013/09/19 16:22:32, David waters wrote: > Do vararg parameters in c# allow passing in lists? or just array? if lists you > might want to > > if(!HasIncomingChagnes(DefaultRepo)) > ... > > private bool HasIncomingChanges(Hg... repos) > https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:168: CreateNuGetPackages(); On 2013/09/19 16:22:32, David waters wrote: > CreateCoreNuGetPackages ? Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:170: } Actually no necessary cause it will be printed by the HasIcomingChanges and by BuildDefaultRepository if they fail On 2013/09/19 16:22:32, David waters wrote: > Good to have else clauses for both these if statments > > else { > user.Tell("Failed to build default Pacakge"); > } else { > user.Tell("Cant build with outstanding changes"); > } > > Please disregard if the methods already warn the user before returning false. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:186: if (CanContinue()) On 2013/09/19 16:22:32, David waters wrote: > Consider:I would reverse this if condition to reduce nesting. > > if(!CanContinue()) > { > return; > } > > > Dito HasIncomingChange Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:188: SamplesRepository = new Hg(new Uri(string.Format(CloneUrl, ".samples")), "samples"); it's actually not null. because that's done before checking in which step we are On 2013/09/19 16:22:32, David waters wrote: > Is constructing new Hg expensive? > If not it may be worth doing > > private void Run() { > initAllRepos(); > if (step 1) ... > > Currently currently DefaultRepository is null so > HasIncomingChanges(AllRepositories) will not check for incoming changes in > DefaultRepo. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:255: input = Console.ReadLine(); On 2013/09/19 16:22:32, David waters wrote: > consider: > input = Console.ReadLine().toLowerCase(); Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Program.cs:347: #region [RELEASE_VERSION]/Generated/Bin In general I agree with you, but because it's only a tool and not our core library, I'll allow myself to make my life easier here, unless you have a strong opinion.... On 2013/09/19 16:22:32, David waters wrote: > Consider:I would be inclinded to break these #regions out into methods. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/ProjectExtenstions.cs (right): https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/ProjectExtenstions.cs:28: /// <summary>Project extensions.</summary> On 2013/09/19 16:22:32, David waters wrote: > Better doc please. > maybe just > > Extension methods for the Project class > > but if you could add a little detail about what sort of functionality you are > added it would be good. Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/ProjectExtenstions.cs:29: static class ProjectExtenstions http://msdn.microsoft.com/en-us/library/ms173121.aspx Internal by the way. Done On 2013/09/19 16:22:32, David waters wrote: > can this class be internal? ( I never remember what the default vis in c# is.) https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... File Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs (right): https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs:22: /// <summary>Updates the Downloads wiki page. </summary> On 2013/09/19 16:22:32, David waters wrote: > please give live url to page Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs:30: { On 2013/09/19 16:22:32, David waters wrote: > consider : > workingDirectory.ThrowIfNullOrEmpty() > dito oldVersion & newVersion Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/... Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs:72: Func<string, string> replaceFunc) On 2013/09/19 16:22:32, David waters wrote: > Consider, > This feels like over-generalization we have twice as many overloads as calls! :) > > I would drop this overload and drop the func. Done. https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Utils/Tr... File Tools/Google.Apis.Utils/TraceExtensions.cs (right): https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Utils/Tr... Tools/Google.Apis.Utils/TraceExtensions.cs:27: /// <summary>Traces the event using the current thread id.</summary> On 2013/09/19 16:22:32, David waters wrote: > Consider: > I no longer like making extension methods public, cause I did early in this > project and now regrate it. > > Either keep internal or maybe move into its own namespace so no-one starts using > this method by accident (inteli-sense). Done.
Sign in to reply to this message.
Sorry Eyal, I'm not going to get a chance to look at this this week, Perf + EOQ => Me very busy. I'm on leave next week, so I would advise trying to get another review to get your Looks Good To Them. Sorry once more, David. On 20 September 2013 19:56, <[email protected]> wrote: > Thanks for the review! > > Let me know if don't have time for last review, so I'll just push it. > > I've not changed a lot, so I'm feeling comfortable to push it without > your LGTM. > > Let me know, have a great wkd! > Eyal > > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.cs<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs> > File Tools/Google.Apis.Release/**Program.cs (right): > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode95<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode95> > Tools/Google.Apis.Release/**Program.cs:95: private Hg DefaultRepository { > get; set; } > I changed it a little bit. > It does the job, and we will focus later on, on moving to github so we > may remove this piece of code > > On 2013/09/19 16:22:32, David waters wrote: > >> If Hg has not changed since I last looked at it ( about a year ago ) >> > it is quite > >> rubish. >> You may wish to do a review of it before using it more. >> > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode120<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode120> > Tools/Google.Apis.Release/**Program.cs:120: > On 2013/09/19 16:22:32, David waters wrote: > >> if(options.Step > 2 || < 1) >> user.tell("Dude! got {0} expecting 1 or 2 "); >> > > Done. > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode157<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode157> > Tools/Google.Apis.Release/**Program.cs:157: if (options.Step == 1) > On 2013/09/19 16:22:32, David waters wrote: > >> Consider >> if(step 1) >> { >> doBaseStep(); // Or better name >> } else if(step 2) { >> doApiSteps(); // Or better name >> } else { >> user.tell("How the hell did you get here?"); >> } >> > > Done. > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode164<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode164> > Tools/Google.Apis.Release/**Program.cs:164: if (!HasIncomingChanges(new > List<Hg> { DefaultRepository })) > I hate vargas. I prefer enumerable. That's my style :) enumerable is > very easy to use and it supports linq :) > > On 2013/09/19 16:22:32, David waters wrote: > >> Do vararg parameters in c# allow passing in lists? or just array? if >> > lists you > >> might want to >> > > if(!HasIncomingChagnes(**DefaultRepo)) >> ... >> > > private bool HasIncomingChanges(Hg... repos) >> > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode168<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode168> > Tools/Google.Apis.Release/**Program.cs:168: CreateNuGetPackages(); > On 2013/09/19 16:22:32, David waters wrote: > >> CreateCoreNuGetPackages ? >> > > Done. > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode170<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode170> > Tools/Google.Apis.Release/**Program.cs:170: } > Actually no necessary cause it will be printed by the HasIcomingChanges > and by BuildDefaultRepository if they fail > > On 2013/09/19 16:22:32, David waters wrote: > >> Good to have else clauses for both these if statments >> > > else { >> user.Tell("Failed to build default Pacakge"); >> } else { >> user.Tell("Cant build with outstanding changes"); >> } >> > > Please disregard if the methods already warn the user before returning >> > false. > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode186<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode186> > Tools/Google.Apis.Release/**Program.cs:186: if (CanContinue()) > On 2013/09/19 16:22:32, David waters wrote: > >> Consider:I would reverse this if condition to reduce nesting. >> > > if(!CanContinue()) >> { >> return; >> } >> > > > Dito HasIncomingChange >> > > Done. > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode188<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode188> > Tools/Google.Apis.Release/**Program.cs:188: SamplesRepository = new Hg(new > Uri(string.Format(CloneUrl, ".samples")), "samples"); > it's actually not null. > because that's done before checking in which step we are > > On 2013/09/19 16:22:32, David waters wrote: > >> Is constructing new Hg expensive? >> If not it may be worth doing >> > > private void Run() { >> initAllRepos(); >> if (step 1) ... >> > > Currently currently DefaultRepository is null so >> HasIncomingChanges(**AllRepositories) will not check for incoming >> > changes in > >> DefaultRepo. >> > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode255<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode255> > Tools/Google.Apis.Release/**Program.cs:255: input = Console.ReadLine(); > On 2013/09/19 16:22:32, David waters wrote: > >> consider: >> input = Console.ReadLine().**toLowerCase(); >> > > Done. > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Program.**cs#newcode347<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode347> > Tools/Google.Apis.Release/**Program.cs:347: #region > [RELEASE_VERSION]/Generated/**Bin > In general I agree with you, but because it's only a tool and not our > core library, I'll allow myself to make my life easier here, unless you > have a strong opinion.... > > On 2013/09/19 16:22:32, David waters wrote: > >> Consider:I would be inclinded to break these #regions out into >> > methods. > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/**ProjectExtenstions.cs<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/ProjectExtenstions.cs> > File Tools/Google.Apis.Release/**ProjectExtenstions.cs (right): > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/**ProjectExtenstions.cs#**newcode28<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/ProjectExtenstions.cs#newcode28> > Tools/Google.Apis.Release/**ProjectExtenstions.cs:28: /// <summary>Project > extensions.</summary> > On 2013/09/19 16:22:32, David waters wrote: > >> Better doc please. >> maybe just >> > > Extension methods for the Project class >> > > but if you could add a little detail about what sort of functionality >> > you are > >> added it would be good. >> > > Done. > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/**ProjectExtenstions.cs#**newcode29<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/ProjectExtenstions.cs#newcode29> > Tools/Google.Apis.Release/**ProjectExtenstions.cs:29: static class > ProjectExtenstions > http://msdn.microsoft.com/en-**us/library/ms173121.aspx<http://msdn.microsoft... > Internal by the way. > Done > > On 2013/09/19 16:22:32, David waters wrote: > >> can this class be internal? ( I never remember what the default vis in >> > c# is.) > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Wiki/**DownloadsPageUpdater.cs<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs> > File Tools/Google.Apis.Release/**Wiki/DownloadsPageUpdater.cs (right): > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Wiki/**DownloadsPageUpdater.cs#**newcode22<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs#newcode22> > Tools/Google.Apis.Release/**Wiki/DownloadsPageUpdater.cs:**22: /// > <summary>Updates the Downloads wiki page. </summary> > On 2013/09/19 16:22:32, David waters wrote: > >> please give live url to page >> > > Done. > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Wiki/**DownloadsPageUpdater.cs#**newcode30<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs#newcode30> > Tools/Google.Apis.Release/**Wiki/DownloadsPageUpdater.cs:**30: { > > On 2013/09/19 16:22:32, David waters wrote: > >> consider : >> workingDirectory.**ThrowIfNullOrEmpty() >> dito oldVersion & newVersion >> > > Done. > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Release/Wiki/**DownloadsPageUpdater.cs#**newcode72<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs#newcode72> > Tools/Google.Apis.Release/**Wiki/DownloadsPageUpdater.cs:**72: > Func<string, > string> replaceFunc) > On 2013/09/19 16:22:32, David waters wrote: > >> Consider, >> This feels like over-generalization we have twice as many overloads as >> > calls! :) > > I would drop this overload and drop the func. >> > > Done. > > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Utils/**TraceExtensions.cs<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Utils/TraceExtensions.cs> > File Tools/Google.Apis.Utils/**TraceExtensions.cs (right): > > https://codereview.appspot.**com/12767046/diff/44001/Tools/** > Google.Apis.Utils/**TraceExtensions.cs#newcode27<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Utils/TraceExtensions.cs#newcode27> > Tools/Google.Apis.Utils/**TraceExtensions.cs:27: /// <summary>Traces the > event using the current thread id.</summary> > On 2013/09/19 16:22:32, David waters wrote: > >> Consider: >> I no longer like making extension methods public, cause I did early in >> > this > >> project and now regrate it. >> > > Either keep internal or maybe move into its own namespace so no-one >> > starts using > >> this method by accident (inteli-sense). >> > > Done. > > https://codereview.appspot.**com/12767046/<https://codereview.appspot.com/127... >
Sign in to reply to this message.
It's OK. I think I'll review it again, and I'll just commit it. 'Thanks! On Sep 25, 2013 5:35 AM, "David W" <[email protected]> wrote: > Sorry Eyal, > I'm not going to get a chance to look at this this week, Perf + EOQ => Me > very busy. I'm on leave next week, so I would advise trying to get another > review to get your Looks Good To Them. > > Sorry once more, > David. > > > On 20 September 2013 19:56, <[email protected]> wrote: > >> Thanks for the review! >> >> Let me know if don't have time for last review, so I'll just push it. >> >> I've not changed a lot, so I'm feeling comfortable to push it without >> your LGTM. >> >> Let me know, have a great wkd! >> Eyal >> >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.cs<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs> >> File Tools/Google.Apis.Release/**Program.cs (right): >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode95<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode95> >> Tools/Google.Apis.Release/**Program.cs:95: private Hg DefaultRepository { >> get; set; } >> I changed it a little bit. >> It does the job, and we will focus later on, on moving to github so we >> may remove this piece of code >> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> If Hg has not changed since I last looked at it ( about a year ago ) >>> >> it is quite >> >>> rubish. >>> You may wish to do a review of it before using it more. >>> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode120<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode120> >> Tools/Google.Apis.Release/**Program.cs:120: >> On 2013/09/19 16:22:32, David waters wrote: >> >>> if(options.Step > 2 || < 1) >>> user.tell("Dude! got {0} expecting 1 or 2 "); >>> >> >> Done. >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode157<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode157> >> Tools/Google.Apis.Release/**Program.cs:157: if (options.Step == 1) >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Consider >>> if(step 1) >>> { >>> doBaseStep(); // Or better name >>> } else if(step 2) { >>> doApiSteps(); // Or better name >>> } else { >>> user.tell("How the hell did you get here?"); >>> } >>> >> >> Done. >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode164<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode164> >> Tools/Google.Apis.Release/**Program.cs:164: if (!HasIncomingChanges(new >> List<Hg> { DefaultRepository })) >> I hate vargas. I prefer enumerable. That's my style :) enumerable is >> very easy to use and it supports linq :) >> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Do vararg parameters in c# allow passing in lists? or just array? if >>> >> lists you >> >>> might want to >>> >> >> if(!HasIncomingChagnes(**DefaultRepo)) >>> ... >>> >> >> private bool HasIncomingChanges(Hg... repos) >>> >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode168<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode168> >> Tools/Google.Apis.Release/**Program.cs:168: CreateNuGetPackages(); >> On 2013/09/19 16:22:32, David waters wrote: >> >>> CreateCoreNuGetPackages ? >>> >> >> Done. >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode170<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode170> >> Tools/Google.Apis.Release/**Program.cs:170: } >> Actually no necessary cause it will be printed by the HasIcomingChanges >> and by BuildDefaultRepository if they fail >> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Good to have else clauses for both these if statments >>> >> >> else { >>> user.Tell("Failed to build default Pacakge"); >>> } else { >>> user.Tell("Cant build with outstanding changes"); >>> } >>> >> >> Please disregard if the methods already warn the user before returning >>> >> false. >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode186<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode186> >> Tools/Google.Apis.Release/**Program.cs:186: if (CanContinue()) >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Consider:I would reverse this if condition to reduce nesting. >>> >> >> if(!CanContinue()) >>> { >>> return; >>> } >>> >> >> >> Dito HasIncomingChange >>> >> >> Done. >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode188<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode188> >> Tools/Google.Apis.Release/**Program.cs:188: SamplesRepository = new >> Hg(new >> Uri(string.Format(CloneUrl, ".samples")), "samples"); >> it's actually not null. >> because that's done before checking in which step we are >> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Is constructing new Hg expensive? >>> If not it may be worth doing >>> >> >> private void Run() { >>> initAllRepos(); >>> if (step 1) ... >>> >> >> Currently currently DefaultRepository is null so >>> HasIncomingChanges(**AllRepositories) will not check for incoming >>> >> changes in >> >>> DefaultRepo. >>> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode255<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode255> >> Tools/Google.Apis.Release/**Program.cs:255: input = Console.ReadLine(); >> On 2013/09/19 16:22:32, David waters wrote: >> >>> consider: >>> input = Console.ReadLine().**toLowerCase(); >>> >> >> Done. >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Program.**cs#newcode347<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Program.cs#newcode347> >> Tools/Google.Apis.Release/**Program.cs:347: #region >> [RELEASE_VERSION]/Generated/**Bin >> In general I agree with you, but because it's only a tool and not our >> core library, I'll allow myself to make my life easier here, unless you >> have a strong opinion.... >> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Consider:I would be inclinded to break these #regions out into >>> >> methods. >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/**ProjectExtenstions.cs<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/ProjectExtenstions.cs> >> File Tools/Google.Apis.Release/**ProjectExtenstions.cs (right): >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/**ProjectExtenstions.cs#**newcode28<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/ProjectExtenstions.cs#newcode28> >> Tools/Google.Apis.Release/**ProjectExtenstions.cs:28: /// >> <summary>Project >> extensions.</summary> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Better doc please. >>> maybe just >>> >> >> Extension methods for the Project class >>> >> >> but if you could add a little detail about what sort of functionality >>> >> you are >> >>> added it would be good. >>> >> >> Done. >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/**ProjectExtenstions.cs#**newcode29<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/ProjectExtenstions.cs#newcode29> >> Tools/Google.Apis.Release/**ProjectExtenstions.cs:29: static class >> ProjectExtenstions >> http://msdn.microsoft.com/en-**us/library/ms173121.aspx<http://msdn.microsoft... >> Internal by the way. >> Done >> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> can this class be internal? ( I never remember what the default vis in >>> >> c# is.) >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Wiki/**DownloadsPageUpdater.cs<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs> >> File Tools/Google.Apis.Release/**Wiki/DownloadsPageUpdater.cs (right): >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Wiki/**DownloadsPageUpdater.cs#**newcode22<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs#newcode22> >> Tools/Google.Apis.Release/**Wiki/DownloadsPageUpdater.cs:**22: /// >> <summary>Updates the Downloads wiki page. </summary> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> please give live url to page >>> >> >> Done. >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Wiki/**DownloadsPageUpdater.cs#**newcode30<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs#newcode30> >> Tools/Google.Apis.Release/**Wiki/DownloadsPageUpdater.cs:**30: { >> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> consider : >>> workingDirectory.**ThrowIfNullOrEmpty() >>> dito oldVersion & newVersion >>> >> >> Done. >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Release/Wiki/**DownloadsPageUpdater.cs#**newcode72<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Release/Wiki/DownloadsPageUpdater.cs#newcode72> >> Tools/Google.Apis.Release/**Wiki/DownloadsPageUpdater.cs:**72: >> Func<string, >> string> replaceFunc) >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Consider, >>> This feels like over-generalization we have twice as many overloads as >>> >> calls! :) >> >> I would drop this overload and drop the func. >>> >> >> Done. >> >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Utils/**TraceExtensions.cs<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Utils/TraceExtensions.cs> >> File Tools/Google.Apis.Utils/**TraceExtensions.cs (right): >> >> https://codereview.appspot.**com/12767046/diff/44001/Tools/** >> Google.Apis.Utils/**TraceExtensions.cs#newcode27<https://codereview.appspot.com/12767046/diff/44001/Tools/Google.Apis.Utils/TraceExtensions.cs#newcode27> >> Tools/Google.Apis.Utils/**TraceExtensions.cs:27: /// <summary>Traces the >> event using the current thread id.</summary> >> On 2013/09/19 16:22:32, David waters wrote: >> >>> Consider: >>> I no longer like making extension methods public, cause I did early in >>> >> this >> >>> project and now regrate it. >>> >> >> Either keep internal or maybe move into its own namespace so no-one >>> >> starts using >> >>> this method by accident (inteli-sense). >>> >> >> Done. >> >> https://codereview.appspot.**com/12767046/<https://codereview.appspot.com/127... >> > >
Sign in to reply to this message.
|