Issue 377: New build for releasing a new version

Issue 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

[peleyal, 2013-09-01 22:31:03]

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

[peleyal, 2013-09-05 15:08:22]

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.

[peleyal, 2013-09-06 13:34:16]

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]

[peleyal, 2013-09-14 21:15:14]

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

[peleyal, 2013-09-16 12:33:53]

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.

[peleyal, 2013-09-16 13:32:13]

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

[David waters, 2013-09-19 16:22:31]

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).

[peleyal, 2013-09-20 18:56:17]

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.

[David waters, 2013-09-25 09:35:54]

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, <peleyal@google.com> 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...
>

[peleyal, 2013-09-25 12:16:38]

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" <davidwaters@google.com> 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, <peleyal@google.com> 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...
>>
>
>