Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(152)

Issue 12662047: Issue 376: Generate NuGet pacakges for generated APIs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by peleyal
Modified:
10 years, 7 months ago
Reviewers:
David waters
CC:
google-api-dotnet-client_googlegroups.com, ngmiceli
Base URL:
https://google-api-dotnet-client.googlecode.com/hg/
Visibility:
Public.

Description

NuGet Publisher

Patch Set 1 #

Patch Set 2 : Improving the publisher (it supports creating nupkg and publishing to NuGet repository) #

Patch Set 3 : add comments #

Patch Set 4 : self review #

Patch Set 5 : minor #

Patch Set 6 : add NuGet.exe file #

Total comments: 105

Patch Set 7 : David review #

Patch Set 8 : minor #

Patch Set 9 : improvements while using the publisher for release 1.5.0-beta #

Total comments: 22

Patch Set 10 : David 2nd review #

Patch Set 11 : minor #

Patch Set 12 : minor || #

Patch Set 13 : second round (running it again, before 1.5.0-beta) #

Patch Set 14 : Run it as production #

Patch Set 15 : minor #

Patch Set 16 : David comments #

Patch Set 17 : minor #

Patch Set 18 : minor #

Patch Set 19 : minor #

Total comments: 20

Patch Set 20 : David final comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1534 lines, -0 lines) Patch
A Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.Config View 1 chunk +6 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.exe View 1 2 3 4 5 Binary file 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.targets View 1 chunk +133 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/App.config View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +36 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +78 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Discovery/DiscoveryDoc.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +26 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Discovery/DiscoveryItem.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +35 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Discovery/DiscoveryService.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +40 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher.csproj View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +90 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetApiPublisher.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +241 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +103 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +267 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +36 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/TraceExtensions.cs View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +33 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config View 1 1 chunk +7 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/NuGet.Publisher.sln View 1 chunk +27 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +16 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.exe View 1 Binary file 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.targets View 1 chunk +133 lines, -0 lines 0 comments Download
A Tools/Google.Apis.NuGet.Publisher/Template/packages/Microsoft.Bcl.Build.1.0.8/tools/Microsoft.Bcl.Build.targets View 1 1 chunk +227 lines, -0 lines 0 comments Download

Messages

Total messages: 8
David waters
Have not got all the way through, here are my first set of comments. https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.targets ...
10 years, 7 months ago (2013-08-14 17:07:07 UTC) #1
peleyal
Thanks so much David! https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.targets File Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.targets (right): https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.targets#newcode12 Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.targets:12: <!-- Determines if package restore ...
10 years, 7 months ago (2013-08-14 21:05:24 UTC) #2
David waters
Next round of comments. It is a personal preference and I would not hold back ...
10 years, 7 months ago (2013-08-16 10:18:36 UTC) #3
peleyal
I think it's ready, but you're the one who need to approve that :) Eyal ...
10 years, 7 months ago (2013-08-16 13:57:43 UTC) #4
David waters
Nearly there! https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs (right): https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs#newcode17 Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:17: namespace Google.Apis.NuGet.Publisher Not really. If there is ...
10 years, 7 months ago (2013-08-19 22:16:36 UTC) #5
peleyal
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs (right): https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs#newcode17 Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:17: namespace Google.Apis.NuGet.Publisher It's something that was very common in ...
10 years, 7 months ago (2013-08-20 16:17:02 UTC) #6
David waters
LGTM A few more comments but I will leave it to you to chose to ...
10 years, 7 months ago (2013-08-27 18:20:11 UTC) #7
peleyal
10 years, 7 months ago (2013-08-27 19:54:59 UTC) #8
Thanks David!
I'm closing this issue and I'll push it in the next few minutes.

Eyal

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs
(right):

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:35:
/// <summary>A NuGet publisher which publishes individual Google API package to
NuGet main repository.</summary>
On 2013/08/27 18:20:12, David waters wrote:
> s/package/packages

Done.

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:35:
/// <summary>A NuGet publisher which publishes individual Google API package to
NuGet main repository.</summary>
On 2013/08/27 18:20:12, David waters wrote:
> s/A nuget publisher which publishes/Publishes

Done.

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs
(right):

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs:40:
// if the destination directory doesn't exist, create it.
On 2013/08/27 18:20:12, David waters wrote:
> Do you want to ClearOrCreateDirectory the Destination directory first?

Done.

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Discovery/DiscoveryDoc.cs
(right):

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Discovery/DiscoveryDoc.cs:20:
public class DiscoveryDoc
I'll leave it in separate files. No special reason, both of the suggestion look
OK to me :)

On 2013/08/27 18:20:12, David waters wrote:
> Totaly up to you:
> Do you want to include the DiscoveryItem class in this file?
> Either as inner class or top level class.
> 
> Or all 3 in DiscoveryService.

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs
(right):

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs:31:
static readonly TraceSource TraceSource = new TraceSource("Google.Apis");
I leave it with Google.Apis cause right now I don't need a different source for
each class. If it was a bigger system I would do that, but not for this small
application.

On 2013/08/27 18:20:12, David waters wrote:
> Does this TraceSorouce want to be more specific? e.g. Google.Apis.Utils or
> Google.Apis.Utils.NuGetUtilites ?
> 
> (I know nothing about TraceSource.)

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs:31:
static readonly TraceSource TraceSource = new TraceSource("Google.Apis");
On 2013/08/27 18:20:12, David waters wrote:
> private?

Done.

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs:34:
public const string LocalNuGetPackageFolder = @"C:\LocalNuGetFeed";
On 2013/08/27 18:20:12, David waters wrote:
> I still not a fan of this. "c:\Foo"
> I have worked in organisations where the standard image has no C drive.
(System
> partition was on D:, because they could).
> Also where writing to the root of the drive was forbidden.
> 
> I'll leave it to you but you might want to consider parameter to override this
> string or
> Using one of the %DIRECTORY% e.g. %LOCAL_PROFILE%/Foo or
> using a tempory file.

Done.

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs:79:
PackageServer ps = new PackageServer("https://nuget.org/", "Google.Apis");
On 2013/08/27 18:20:12, David waters wrote:
> longer variable name please. (aberrations discouraged)

Done.

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs
(right):

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs:25:
public static class Utilities
On 2013/08/27 18:20:12, David waters wrote:
> public?
> Is there a reason this is not assembly private? Is this shipped in one of
> customer facing DLLs? If so I would really recommend against this.
> 
> I have strongly regretted making extension methods public in customer facing
> DLLs (string.isNullOrEmpty() etc).

Done.

https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.P...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs:25:
public static class Utilities
On 2013/08/27 18:20:12, David waters wrote:
> Utils.Utilities is a pretty meaningless name. 
> 
> TraceExtensions?

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b