Issue 12662047: Issue 376: Generate NuGet pacakges for generated APIs

	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

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

11 years, 2 months ago (2013-08-14 21:05:24 UTC) #2

Thanks so much David!

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.targets (right):

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/.nuget/NuGet.targets:12: <!-- Determines if
package restore consent is required to restore packages -->
ACK. (generated)
On 2013/08/14 17:07:08, David waters wrote:
> if this file is hand edited please shorten line length.
> If generated no action required.

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

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:33:
public class ApiItemLogic
ApiGenerator ?
ApiPackager ?
ApiNugetPublisher ? (sound the best one so far, but I'm the worst with names)

On 2013/08/14 17:07:08, David waters wrote:
> Logic is a bit of a strange name as evidenced by "constructs a new logic
item".
> Can you think of a better name?

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:39:
private const int NuGetPushTimeout = 10000;
On 2013/08/14 17:07:08, David waters wrote:
> Comment about units, or better use a TimeSpan and convert when passing to what
> ever uses this.
> Alternative units in field name.

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:41:
/// <summary>Gets or sets the working directory which the source will be
downloaded to.</summary>
I saw that the framework classes has this pattern "Gets or sets" so I'm doing
the same.
On 2013/08/14 17:07:08, David waters wrote:
> Gets or sets feels very redundent here. (and below).
>

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:45:
public string DownloadUriFormat { get; set; }
On 2013/08/14 17:07:08, David waters wrote:
> Your mixing URL in the comments with URI in the variable name, which do you
> mean, these are different.

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:45:
public string DownloadUriFormat { get; set; }
Remember that this is a Tool. Not used in the generated API or on runtime.
I don't think that I need to cover and check input parameters.
What do you think?
On 2013/08/14 17:07:08, David waters wrote:
> Is this for downloading the existing NuGet package? or the discovery?
> Could you please document and enforce your expectations of this string. e.g.
> this will will be formated with two parameters the first being XX and the
second
> being YY. If someone assign a string with > 2 substitutions you will get an
> exception latter.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:54:
/// <summary>The regex version to extract the client version and the specific
API revision.</summary>
On 2013/08/14 17:07:08, David waters wrote:
> The regex to extract ...
> 
> ?

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:54:
/// <summary>The regex version to extract the client version and the specific
API revision.</summary>
On 2013/08/14 17:07:08, David waters wrote:
> where does this extract from?

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:57:
/// <summary>The main NuGet repository is used to publish a new
package.</summary>
On 2013/08/14 17:07:08, David waters wrote:
> The main NuGet repository. This is used to ...

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:58:
private static readonly IPackageRepository NuGetRepository =
On 2013/08/14 17:07:08, David waters wrote:
> maybe change name to MainNuGet... or LiveNuGet... or public or production

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:61:
/// <summary>The local NuGet package format to store the package
locally</summary>
On 2013/08/14 17:07:08, David waters wrote:
> try something like:
> A string format for constructing the path to the local NuGet package.
> 
> Mention how many parameters this expects (1) and what it is (The api name)

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:70:
/// <summary>The main logic contains the following:
On 2013/08/14 17:07:08, David waters wrote:
> Does this relate to this method?
> 
> "This is the main logic, which does the following:"

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:72:
/// <item><description>Download the API's bundle</description></item>
"Each item in the list is specified with an <item> block. When creating a
definition list, you will need to specify both term and description. However,
for a table, bulleted list, or numbered list, you only need to supply an entry
for description."

On 2013/08/14 17:07:08, David waters wrote:
> is description required here?

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:73:
/// <item><description>Extract the API's sources, in case it's new version of
the API</description></item>
On 2013/08/14 17:07:08, David waters wrote:
> in case it __is a___ new version ... ?
> Even with this change this item does not make sense. please elaborate. 

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:74:
/// <item><description>Build</description></item>
On 2013/08/14 17:07:08, David waters wrote:
> build the api from the latest published discovery document.

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:83:

On 2013/08/14 17:07:08, David waters wrote:
> Preference:I would prefer all these vars to be explicity typed.
> 
> from reading
> var zipFile = await DownloadBundle();
> var sourceFolder = ExtractSources(zipFile);
> 
> I have no idea of the types of these variable.

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:98:
}
no need I'm writing that in ExtractSources
On 2013/08/14 17:07:08, David waters wrote:
> else {
>   TraceSource.TraceEvent("Did no action due to no sourceFoler")
> }

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:124:
private async Task<string> DownloadBundle()
I can call to async method (like I'm doing here by calling to "await
client.DownloadFileTaskAsync(new Uri(uri), outputFile);")
only if the method itself is defined as async.

that's why I'm declaring this one as async.
I can explain further if you would like, just let me know

On 2013/08/14 17:07:08, David waters wrote:
> if your one consumer of this method awaits the result why is this 
> private async Task<string>
> instead of 
> private string
> 
> [Note I have not worked with await,Task etc in c# so may well be wrong,
applies
> to other comments as well.]

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:124:
private async Task<string> DownloadBundle()
On 2013/08/14 17:07:08, David waters wrote:
> why not Task<FileInfo>?
> otherwise reading the signature I was unsure if this returned the contents of
> the file or the path to the file.

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:143:
/// <returns>The source folder. <c>null</c> if a NuGet package already exists
for this bundle version</returns>
when I'm writing bundle in all cases I mean the specific API bundle you can
download for example from:
https://developers.google.com/resources/api-libraries/download/datastore/v1be...
https://developers.google.com/resources/api-libraries/download/plus/v1/csharp
...

On 2013/08/14 17:07:08, David waters wrote:
> I don't know what you mean by bundle here.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:144:
private string ExtractSources(string zipFile)
changed the comment
On 2013/08/14 17:07:08, David waters wrote:
> private FileInfo ExtractSource(FileInfo zipFile)

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:144:
private string ExtractSources(string zipFile)
I can extract to two methods, but I think it's ok that it's one method, because
it only extracts the sources.

It actually extracts zip
then check if a nuget package already exists
If no - extract the sources.

I'll make the comment clearer

On 2013/08/14 17:07:08, David waters wrote:
> This method is doing two things and its name does not reflect that.
> First extracting the zip file, second checking for the existance of the nuget
> package.
> 
> Can this be broken into two methods or the second part pulled up to the run
> method?

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:157:
var beta = version.IndexOf("-beta");
Agree with you, but I'm using this beta variable later on.
I don't want to have two parameters
var beta = ...
var isBeta = veta != -1

On 2013/08/14 17:07:08, David waters wrote:
> would prefer bool isBeta == version... >= 0

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:168:
// If a NuGet package already exists with this version, which contains the
library version (e.g. 1.5.0) and
the core Google.Apis .NET client version.
Is it better?
On 2013/08/14 17:07:08, David waters wrote:
> By library version, do you mean the version of the Google-api-dotnet-client or
> the public version of the api e.g "Maps v1.2"?

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:170:
bool exists = IsNugetPackageExist(item, version);
On 2013/08/14 17:07:08, David waters wrote:
> would suggest
>  DoesNugetPackageExists(...)
> or 
>  NugetPackageExists

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:173:
TraceSource.TraceEvent(TraceEventType.Information, "{0}\t NUGET PACKAGE ALREADY
EXISTS", item);
On 2013/08/14 17:07:08, David waters wrote:
> all caps is shouty, yet you are still logging as information.
> Either it is bad and we should shout at error level.
> Or this is fine and we should not shout at information level.

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:192:
/// project file
On 2013/08/14 17:07:08, David waters wrote:
> full stop at end of comment ( here and other places)

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:197:
var buildDirectory = TemplateDirectory + @"\Build";
I don't really care on Mono with this utility cause Mono can't compile a PCL
project
On 2013/08/14 17:07:08, David waters wrote:
> Would prefer this to be DirectoryInfo manipulation not string manipulation.
(or
> Path.combine)
> Does mono hide the \ / difference in linux?
> 
> DirectoryInfo buildDirectory = new DirectoryInfo(Path.combine(TemplageDir,
> "Build"));
> if(buildDirectory.Exists)

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:204:
var projectFile = Directory.GetFiles(buildDirectory,
"*.csproj").FirstOrDefault();
On 2013/08/14 17:07:08, David waters wrote:
> Why firstorDefault here, if no project do we not want to throw?

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:205:
TraceSource.TraceEvent(TraceEventType.Information, "{0}\t Building \"{1}\"",
item, projectFile);
No need I changed to Single()

I must admit that I'm happy with your comment. Thanks!

On 2013/08/14 17:07:08, David waters wrote:
> if projectFile == null
>   throw new exception ?

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:244:
TraceSource.TraceEvent(TraceEventType.Information, "{0}\t Pushing to NuGet
repository", item);
BEfore calling to this method I'm checking if the key is null or empty
On 2013/08/14 17:07:08, David waters wrote:
> As you advise not passin gin a NugetApiKet, is it worth checking for its
> existence not just relying on exception?

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:262:
private string CreateLocalNupkgFile(string buildDirectory)
I preferred to work with file path because I don't need actually anything else.
I change the names to Path when necessary

On 2013/08/14 17:07:08, David waters wrote:
> DirectoryInfo

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:279:
TraceSource.TraceEvent(TraceEventType.Information, "{0}\t {1} file was created",
item, nuspec);
I wanted to check how it's going to be working with TraceSource directly.
I actually think now we should support it directly in our core library. cause
it's easy to use and it provide you everything log4net and similar loggers do.
Let's about this one soon :)
I want to hear your thoughts.
On 2013/08/14 17:07:08, David waters wrote:
> Very Optional: A little surprised to see you logging to the traceSource
> directly. The way I would approach this would to use a vanilla logger (log4net
> etc) and have an appender that uses a traceSource.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs
(right):

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:73:
const string DownloadBundleTempDirectory = @"C:\Temp\NuGetPublisher\";
Right now I don't care about linux, because PCL can't be built there.
But you are right - I'll do so
On 2013/08/14 17:07:08, David waters wrote:
> Not System.IO.Path.GetTempPath()?
> 
> Will make it harder to eventuly run on linux.
> 
> (applies to other c:\ in this cl)

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:75: ///
<summary>The download URI format to download a specific API format</summary>
On 2013/08/14 17:07:08, David waters wrote:
> Document the number of parameters and what you hope to be put in.

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:77:
@"https://google-api-client-libraries.appspot.com/resources/api-libraries/download/stable/{0}/{1}/csharp?deps=0";
Man I thought you will give up on that one....
it's a long string :)
On 2013/08/14 17:07:08, David waters wrote:
> line length

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:77:
@"https://google-api-client-libraries.appspot.com/resources/api-libraries/download/stable/{0}/{1}/csharp?deps=0";
On 2013/08/14 17:07:08, David waters wrote:
> Optional: @ is not required for this string.

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:86:
static Program()
TemplateDirectory is readonly so I think it's ok here...

On 2013/08/14 17:07:08, David waters wrote:
> Personal: I dislike static constructors. This could be done in main or a init
> method or a normal constructor.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:89: //
remove
"Google.Apis.NuGet.Publisher\\bin\\{Debug|Release}\\Google.Apis.NuGet.Publisher.exe"
LIKE
On 2013/08/14 17:07:08, David waters wrote:
> Suggestion:
> FileInfo assembly = new FileInfo(...Location);
> DirectoryInfo execDir = assembly.Directory.Parent.Parent.Parent;
> TemplateDirectory = DirectoryInfo(Path.combine(execDir, Template));

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:99: {
On 2013/08/14 17:07:08, David waters wrote:
> Personal Preference:
> my main methods generly look like:
> 
> s v Main(string[] args) {
>   try{
>     Program p = new Programe();
>     p.run(args);
>   }catch(Exception ex) {
>     writeException(ex)
>   }
>   c.writeline(...);
>   c.readline();
> }
> 
> Reasons:
>   a small try catch block for whole programe. 
>   get out of static context quickly. (avoids static constructors which i
> dislike)
>   also allows the same methods to be call by a service ( just construct the
call
> run(options))

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:103: //
TODO(peleyal): add explanation which option is missing
The output contains the full help already but there is no indication what is
exactly wrong.On 2013/08/14 17:07:08, David waters wrote:
> Console.writeline(options.GetHelpText()); 
> 
> for now? could leave the todo if you want better error messages.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:166:
TraceSource.TraceEvent(TraceEventType.Information, "\"{0}\" folder was created",
DownloadBundleTempDirectory);
On 2013/08/14 17:07:08, David waters wrote:
> line length

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:193:
await item.Run();
I want to do one API at time. 

1. The code looks like it procedural although it's not! there is automatic
continuation when something is done. and the thread won't be block!
2. The advantage here, is that our Downloads method will not block the current
thread, they will just return it to the thread pool, and after the web call be
over it will return to the same context.
3. I like async-await :)
On 2013/08/14 17:07:08, David waters wrote:
> Letting my lack of async/await/task knowledge show.
> Does this await allow the for loop to continue? (I hope not)
> If not what advantage do we gain from these methods being async?
> We appare to me to be only processing one Api at a time.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:215:
await item.Test(@"Z:\Shared\NuGet\");
LOL.... 
it should be a parameter :)
On 2013/08/14 17:07:08, David waters wrote:
> Garrhhh you require anyone running this util to have a z directory mapped? Can
> we do better here?
> Like having the full path to the network share (if it is one)
> If it is not a network share can this be relative to the current directory, or
> the assembly directory or a tempory file?

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs
(right):

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs:11:
[assembly: AssemblyCompany("")]
On 2013/08/14 17:07:08, David waters wrote:
> Do you want to put in google here and trademark?

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs:35:
[assembly: AssemblyVersion("1.0.0.0")]
It will be part of 1.5.0 release :)
On 2013/08/14 17:07:08, David waters wrote:
> suggestion: I like having at least one * in the version number.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs
(right):

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs:16: 
On 2013/08/14 17:07:08, David waters wrote:
> Out of time, have not reviewed.

Done. :)

David waters

Next round of comments. It is a personal preference and I would not hold back ...

11 years, 2 months ago (2013-08-16 10:18:36 UTC) #3

Next round of comments.

It is a personal preference and I would not hold back an LGTM for it, but I
would prefer typed classes over strings
Uri > String
FileInfo > String
DirectoryInfo > String
Integer > String

This makes code, parameters, fields far more self documenting.

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

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:33:
public class ApiItemLogic
How about?

///<summary> Publishes individule api packages to nuget.
NugetApiPublisher

On 2013/08/14 21:05:24, peleyal wrote:
> ApiGenerator ?
> ApiPackager ?
> ApiNugetPublisher ? (sound the best one so far, but I'm the worst with names)
> 
> On 2013/08/14 17:07:08, David waters wrote:
> > Logic is a bit of a strange name as evidenced by "constructs a new logic
> item".
> > Can you think of a better name?
>

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:41:
/// <summary>Gets or sets the working directory which the source will be
downloaded to.</summary>
Good reasoning.

On 2013/08/14 21:05:24, peleyal wrote:
> I saw that the framework classes has this pattern "Gets or sets" so I'm doing
> the same.
> On 2013/08/14 17:07:08, David waters wrote:
> > Gets or sets feels very redundent here. (and below).
> > 
>

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:45:
public string DownloadUriFormat { get; set; }
Document at least, I would also enfource on the setter, but up to you.

Remember I happen to know that tomorrow you will be poached for the high impact,
high visibility soooo cool and exiting project you have always dreamed of
working on. Will drop this project like a strained analogy and some other poor
sod is going to have to pick up where you left off.

When they see BundleUri they may not know what it means or what values it
expects. 

On 2013/08/14 21:05:24, peleyal wrote:
> Remember that this is a Tool. Not used in the generated API or on runtime.
> I don't think that I need to cover and check input parameters.
> What do you think?
> On 2013/08/14 17:07:08, David waters wrote:
> > Is this for downloading the existing NuGet package? or the discovery?
> > Could you please document and enforce your expectations of this string. e.g.
> > this will will be formated with two parameters the first being XX and the
> second
> > being YY. If someone assign a string with > 2 substitutions you will get an
> > exception latter.
>

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:72:
/// <item><description>Download the API's bundle</description></item>
On 2013/08/14 17:07:08, David waters wrote:
> Download the API's bundle from ... 
> NuGet?

I think you missed this comment

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:279:
TraceSource.TraceEvent(TraceEventType.Information, "{0}\t {1} file was created",
item, nuspec);
The set up for log4net is more painful then it should be. 
But once there I find the api very easy to use. 
In a lot of ways the less dependencies we have the better. 
I assume you can control the level of logging by Package/Class like you can with
log4Net?
I remember the logging within the framework being abstracted out into our own
very simple interface. Could TraceSource sit behind that?

On 2013/08/14 21:05:24, peleyal wrote:
> I wanted to check how it's going to be working with TraceSource directly.
> I actually think now we should support it directly in our core library. cause
> it's easy to use and it provide you everything log4net and similar loggers do.
> Let's about this one soon :)
> I want to hear your thoughts.
> On 2013/08/14 17:07:08, David waters wrote:
> > Very Optional: A little surprised to see you logging to the traceSource
> > directly. The way I would approach this would to use a vanilla logger
(log4net
> > etc) and have an appender that uses a traceSource.
>

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:54:
HelpText = @"Define the specific language version we will use to download the
bundles")]
I need a bit more help in this comment. Are the bundles Google.api.foo bundels?
Cause I did not think they had langauge versions?

In fact what is a language version in this case? A human speackable language or
are you talking c# 2.5 vs 4.0? in which case is it not the CLR version we are
after.

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:139: //
reoove
"Google.Apis.NuGet.Publisher\\bin\\{Debug|Release}\\Google.Apis.NuGet.Publisher.exe"
typo

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs:25: {
This class appears to have been copied and pasted from the web, please delete
every line and rewrite completely.

We can not have code introduced to our source control system that we do not own
the copywrite of.

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
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.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:17:
namespace Google.Apis.NuGet.Publisher
what are the square brackets "[]" doing in the file path?
Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:20:
public class DiscoveryDoc
Why this class not just the generated data classes?

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryService.cs
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryService.cs:27:
public async Task<IEnumerable<DiscoveryDoc.Item>> GetApis(string discovery)
why string not System.Uri?

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config:3:
<package id="CommandLineParser" version="1.9.71" targetFramework="net45" />
(I don't know enough about nuget)
Why are you specifiing specific versions?
Is it not better to just get the latest version?

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config (right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config:8: <add
key="TestSource" value="C:\LocalNuGetFeed" />
again what looks to be a fairly arbitrary directory. Does this still run with
out this directory (or drive) present?
Where have you documented that this folder is required and what it should look
like?

In general I dislike this sort of thing as it increases the number of steps a
developer needs to take to get this solution compiling and running idally the
steps would be

1 install VS
2 install Source Control
3 fetch solution from source control
4 double click on sln file.

already we have
 1.1 install nuget
does this mean we need
 1.2 create folder c:\LocalNuGetFeed
 1.3 do some magic
?

peleyal

I think it's ready, but you're the one who need to approve that :) Eyal ...

11 years, 2 months ago (2013-08-16 13:57:43 UTC) #4

I think it's ready, but you're the one who need to approve that :)

Eyal

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

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:33:
public class ApiItemLogic
Done. I didn't change the file name yet, because I want you to be able to review
it easily with diff, and not by creating a new file
On 2013/08/16 10:18:36, David waters wrote:
> How about?
> 
> ///<summary> Publishes individule api packages to nuget.
> NugetApiPublisher
> 
> 
> 
> On 2013/08/14 21:05:24, peleyal wrote:
> > ApiGenerator ?
> > ApiPackager ?
> > ApiNugetPublisher ? (sound the best one so far, but I'm the worst with
names)
> > 
> > On 2013/08/14 17:07:08, David waters wrote:
> > > Logic is a bit of a strange name as evidenced by "constructs a new logic
> > item".
> > > Can you think of a better name?
> > 
>

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:41:
/// <summary>Gets or sets the working directory which the source will be
downloaded to.</summary>
ACK.
On 2013/08/16 10:18:36, David waters wrote:
> Good reasoning.
> 
> On 2013/08/14 21:05:24, peleyal wrote:
> > I saw that the framework classes has this pattern "Gets or sets" so I'm
doing
> > the same.
> > On 2013/08/14 17:07:08, David waters wrote:
> > > Gets or sets feels very redundent here. (and below).
> > > 
> > 
>

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:45:
public string DownloadUriFormat { get; set; }
Better?
On 2013/08/16 10:18:36, David waters wrote:
> Document at least, I would also enfource on the setter, but up to you.
> 
> Remember I happen to know that tomorrow you will be poached for the high
impact,
> high visibility soooo cool and exiting project you have always dreamed of
> working on. Will drop this project like a strained analogy and some other poor
> sod is going to have to pick up where you left off.
> 
> When they see BundleUri they may not know what it means or what values it
> expects. 
> 
> On 2013/08/14 21:05:24, peleyal wrote:
> > Remember that this is a Tool. Not used in the generated API or on runtime.
> > I don't think that I need to cover and check input parameters.
> > What do you think?
> > On 2013/08/14 17:07:08, David waters wrote:
> > > Is this for downloading the existing NuGet package? or the discovery?
> > > Could you please document and enforce your expectations of this string.
e.g.
> > > this will will be formated with two parameters the first being XX and the
> > second
> > > being YY. If someone assign a string with > 2 substitutions you will get
an
> > > exception latter.
> > 
>

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:72:
/// <item><description>Download the API's bundle</description></item>
On 2013/08/16 10:18:36, David waters wrote:
> On 2013/08/14 17:07:08, David waters wrote:
> > Download the API's bundle from ... 
> > NuGet?
> 
> I think you missed this comment
> 

Done.

https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:279:
TraceSource.TraceEvent(TraceEventType.Information, "{0}\t {1} file was created",
item, nuspec);
I think that I'm going to change the way we work with ILogger in the library
later on.
I'll may create a log4net adapter but the default will be tracesource. It's very
easy to use, I wanted to check how is it in this tool, and it looks nice!
And yep you can control the level of specific source (class, namespace)

On 2013/08/16 10:18:36, David waters wrote:
> The set up for log4net is more painful then it should be. 
> But once there I find the api very easy to use. 
> In a lot of ways the less dependencies we have the better. 
> I assume you can control the level of logging by Package/Class like you can
with
> log4Net?
> I remember the logging within the framework being abstracted out into our own
> very simple interface. Could TraceSource sit behind that?
>  
> 
> 
> On 2013/08/14 21:05:24, peleyal wrote:
> > I wanted to check how it's going to be working with TraceSource directly.
> > I actually think now we should support it directly in our core library.
cause
> > it's easy to use and it provide you everything log4net and similar loggers
do.
> > Let's about this one soon :)
> > I want to hear your thoughts.
> > On 2013/08/14 17:07:08, David waters wrote:
> > > Very Optional: A little surprised to see you logging to the traceSource
> > > directly. The way I would approach this would to use a vanilla logger
> (log4net
> > > etc) and have an appender that uses a traceSource.
> > 
>

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:54:
HelpText = @"Define the specific language version we will use to download the
bundles")]
On 2013/08/16 10:18:36, David waters wrote:
> I need a bit more help in this comment. Are the bundles Google.api.foo
bundels?
> Cause I did not think they had langauge versions?
> 
> In fact what is a language version in this case? A human speackable language
or
> are you talking c# 2.5 vs 4.0? in which case is it not the CLR version we are
> after.

Done.

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:139: //
reoove
"Google.Apis.NuGet.Publisher\\bin\\{Debug|Release}\\Google.Apis.NuGet.Publisher.exe"
reoove is the new remove :)
On 2013/08/16 10:18:36, David waters wrote:
> typo

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs:25: {
I already changed it completely, but I'll go further with that

On 2013/08/16 10:18:36, David waters wrote:
> This class appears to have been copied and pasted from the web, please delete
> every line and rewrite completely.
> 
> We can not have code introduced to our source control system that we do not
own
> the copywrite of.

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
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.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:17:
namespace Google.Apis.NuGet.Publisher
I used it long time ago, and it was nice. It means that I'm still using the same
namespace as it wasn't in that directory, so it only means a small separation in
the file layout. It could be in the same folder as the [Discovery].

makes sense?

On 2013/08/16 10:18:36, David waters wrote:
> what are the square brackets "[]" doing in the file path?
> Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:20:
public class DiscoveryDoc
?

Do you mean why I'm not using the generated discovery service here? If so, I
don't want to. I just need the list of items, that's all
On 2013/08/16 10:18:36, David waters wrote:
> Why this class not just the generated data classes?

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryService.cs
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryService.cs:27:
public async Task<IEnumerable<DiscoveryDoc.Item>> GetApis(string discovery)
On 2013/08/16 10:18:36, David waters wrote:
> why string not System.Uri?

Done.

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config:3:
<package id="CommandLineParser" version="1.9.71" targetFramework="net45" />
It's how nuget works. You can run 
Get-Package -Updates to get the list of updates you need

see for more details
http://docs.nuget.org/docs/start-here/using-the-package-manager-console

On 2013/08/16 10:18:36, David waters wrote:
> (I don't know enough about nuget)
> Why are you specifiing specific versions?
> Is it not better to just get the latest version?

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config (right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config:8: <add
key="TestSource" value="C:\LocalNuGetFeed" />
On 2013/08/16 10:18:36, David waters wrote:
> again what looks to be a fairly arbitrary directory. Does this still run with
> out this directory (or drive) present?
> Where have you documented that this folder is required and what it should look
> like?
> 
> In general I dislike this sort of thing as it increases the number of steps a
> developer needs to take to get this solution compiling and running idally the
> steps would be
> 
> 1 install VS
> 2 install Source Control
> 3 fetch solution from source control
> 4 double click on sln file.
> 
> already we have
>  1.1 install nuget
> does this mean we need
>  1.2 create folder c:\LocalNuGetFeed
>  1.3 do some magic
> ? 

Done.

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

11 years, 2 months ago (2013-08-19 22:16:36 UTC) #5

Nearly there!

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
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.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:17:
namespace Google.Apis.NuGet.Publisher
Not really.
If there is enough difference in this file to have a new folder "[Discovery]"
then there should be enough difference to have a new namespace. 

There is no compulsion in c# to have the namespace match the directory structure
so I'm not sure what the square brackets are gaining you. 

This seems to be breacking the Nth guildline of development don't surprise the
next coder working on this.

On 2013/08/16 13:57:43, peleyal wrote:
> I used it long time ago, and it was nice. It means that I'm still using the
same
> namespace as it wasn't in that directory, so it only means a small separation
in
> the file layout. It could be in the same folder as the [Discovery].
> 
> makes sense?
> 
> On 2013/08/16 10:18:36, David waters wrote:
> > what are the square brackets "[]" doing in the file path?
> > Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs
>

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:20:
public class DiscoveryDoc
Yes that was the question.
Can you give more justification then you don't want to?
What would the harm in using the generated discovery service do?

I'm inclinded to use standard unless there is a good reason not to. In this case
the generated service should be standard.

On 2013/08/16 13:57:43, peleyal wrote:
> ?
> 
> Do you mean why I'm not using the generated discovery service here? If so, I
> don't want to. I just need the list of items, that's all
> On 2013/08/16 10:18:36, David waters wrote:
> > Why this class not just the generated data classes?
>

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config:3:
<package id="CommandLineParser" version="1.9.71" targetFramework="net45" />
Is it possible not specify the version? 
(If this file was autgenerated/point-and-click-generated probably not an issue)

I have had more experience with Maven which is a similar tool in the Java land,
and within Maven you can depend on a pinned version (v1.2.3) or the latest
version (@snapshot). 

It is almost always better to rely on snapshot, as it makes multi-library
dependancies work so much better. 

On 2013/08/16 13:57:43, peleyal wrote:
> It's how nuget works. You can run 
> Get-Package -Updates to get the list of updates you need
> 
> see for more details
> http://docs.nuget.org/docs/start-here/using-the-package-manager-console
> 
> On 2013/08/16 10:18:36, David waters wrote:
> > (I don't know enough about nuget)
> > Why are you specifiing specific versions?
> > Is it not better to just get the latest version?
>

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

11 years, 2 months ago (2013-08-20 16:17:02 UTC) #6

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
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.Pu...
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 a place that I worked for.
I agree that here it doesn't make sense so I indeed created a Discovery
namespace and directory
On 2013/08/19 22:16:36, David waters wrote:
> Not really.
> If there is enough difference in this file to have a new folder "[Discovery]"
> then there should be enough difference to have a new namespace. 
> 
> There is no compulsion in c# to have the namespace match the directory
structure
> so I'm not sure what the square brackets are gaining you. 
> 
> This seems to be breacking the Nth guildline of development don't surprise the
> next coder working on this.
> 
> On 2013/08/16 13:57:43, peleyal wrote:
> > I used it long time ago, and it was nice. It means that I'm still using the
> same
> > namespace as it wasn't in that directory, so it only means a small
separation
> in
> > the file layout. It could be in the same folder as the [Discovery].
> > 
> > makes sense?
> > 
> > On 2013/08/16 10:18:36, David waters wrote:
> > > what are the square brackets "[]" doing in the file path?
> > > Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs
> > 
>

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs:20:
public class DiscoveryDoc
I really didn't like that in our old generator there was a usage on a generated
discovery service,
because you needed to update that each time. 
Here i have two options:
1. use the nuget package of the discovery service
2. just declare a silly service that will do so.

If you insist I can go with the first option, although I think that we should
keep it simple and choose the 2nd one.

On 2013/08/19 22:16:36, David waters wrote:
> Yes that was the question.
> Can you give more justification then you don't want to?
> What would the harm in using the generated discovery service do?
> 
> I'm inclinded to use standard unless there is a good reason not to. In this
case
> the generated service should be standard.
> 
> On 2013/08/16 13:57:43, peleyal wrote:
> > ?
> > 
> > Do you mean why I'm not using the generated discovery service here? If so, I
> > don't want to. I just need the list of items, that's all
> > On 2013/08/16 10:18:36, David waters wrote:
> > > Why this class not just the generated data classes?
> > 
>

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
File
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config
(right):

https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Pu...
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config:3:
<package id="CommandLineParser" version="1.9.71" targetFramework="net45" />
I'm familiar with maven as well.
Anyway you need to know the version, because the packages (and the dlls) will
live under that directory.
Look for example in the csproj - it contains the exact path to those packages.

Secondly, you can check from time to time for new updates.

On 2013/08/19 22:16:36, David waters wrote:
> Is it possible not specify the version? 
> (If this file was autgenerated/point-and-click-generated probably not an
issue)
> 
> I have had more experience with Maven which is a similar tool in the Java
land,
> and within Maven you can depend on a pinned version (v1.2.3) or the latest
> version (@snapshot). 
> 
> It is almost always better to rely on snapshot, as it makes multi-library
> dependancies work so much better. 
> 
> On 2013/08/16 13:57:43, peleyal wrote:
> > It's how nuget works. You can run 
> > Get-Package -Updates to get the list of updates you need
> > 
> > see for more details
> > http://docs.nuget.org/docs/start-here/using-the-package-manager-console
> > 
> > On 2013/08/16 10:18:36, David waters wrote:
> > > (I don't know enough about nuget)
> > > Why are you specifiing specific versions?
> > > Is it not better to just get the latest version?
> > 
>

David waters

LGTM A few more comments but I will leave it to you to chose to ...

11 years, 2 months ago (2013-08-27 18:20:11 UTC) #7

peleyal

11 years, 2 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.

Expand All Messages | Collapse All Messages

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

Description

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 #

Patch Set 7 : David review #

Patch Set 8 : minor #

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

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 #

Patch Set 20 : David final comments #

Messages