Code review - Issue 12662047: Issue 376: Generate NuGet pacakges for generated APIshttps://codereview.appspot.com/2013-08-27T19:54:59+00:00rietveld
Message from unknown
2013-08-09T22:52:23+00:00peleyalurn:md5:2cb892f470194275c175e23d34d8b18e
Message from unknown
2013-08-13T01:37:36+00:00peleyalurn:md5:eaff5fea05d9d53616f2cf17ae5b981c
Message from unknown
2013-08-14T14:52:33+00:00peleyalurn:md5:ee8e65ec1606114f348cbbdebd5f25fd
Message from unknown
2013-08-14T15:03:02+00:00peleyalurn:md5:564ede20a9a31bf41764800ca5d5db70
Message from unknown
2013-08-14T15:07:48+00:00peleyalurn:md5:039a2c6a060a213da32799cef5b53b0e
Message from unknown
2013-08-14T15:08:39+00:00peleyalurn:md5:109a2f93681e6130d6408dbdf0aedfd1
Message from davidwaters@google.com
2013-08-14T17:07:07+00:00David watersurn:md5:20cbc2748ca2b49166e3515496042b1f
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
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 consent is required to restore packages -->
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs (right):
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode33
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:33: public class ApiItemLogic
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode39
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:39: private const int NuGetPushTimeout = 10000;
Comment about units, or better use a TimeSpan and convert when passing to what ever uses this.
Alternative units in field name.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode41
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>
Gets or sets feels very redundent here. (and below).
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode45
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:45: public string DownloadUriFormat { get; set; }
Your mixing URL in the comments with URI in the variable name, which do you mean, these are different.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode45
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:45: public string DownloadUriFormat { get; set; }
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode54
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>
The regex to extract ...
?
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode54
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>
where does this extract from?
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode57
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>
The main NuGet repository. This is used to ...
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode58
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:58: private static readonly IPackageRepository NuGetRepository =
maybe change name to MainNuGet... or LiveNuGet... or public or production
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode61
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:61: /// <summary>The local NuGet package format to store the package locally</summary>
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)
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode70
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:70: /// <summary>The main logic contains the following:
Does this relate to this method?
"This is the main logic, which does the following:"
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode72
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:72: /// <item><description>Download the API's bundle</description></item>
is description required here?
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode72
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:72: /// <item><description>Download the API's bundle</description></item>
Download the API's bundle from ...
NuGet?
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode73
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>
in case it __is a___ new version ... ?
Even with this change this item does not make sense. please elaborate.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode74
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:74: /// <item><description>Build</description></item>
build the api from the latest published discovery document.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode83
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:83:
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.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode98
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:98: }
else {
TraceSource.TraceEvent("Did no action due to no sourceFoler")
}
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode124
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:124: private async Task<string> DownloadBundle()
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode124
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:124: private async Task<string> DownloadBundle()
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.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode143
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>
I don't know what you mean by bundle here.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode144
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:144: private string ExtractSources(string zipFile)
private FileInfo ExtractSource(FileInfo zipFile)
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode144
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:144: private string ExtractSources(string zipFile)
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode157
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:157: var beta = version.IndexOf("-beta");
would prefer bool isBeta == version... >= 0
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode168
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
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode170
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:170: bool exists = IsNugetPackageExist(item, version);
would suggest
DoesNugetPackageExists(...)
or
NugetPackageExists
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode173
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:173: TraceSource.TraceEvent(TraceEventType.Information, "{0}\t NUGET PACKAGE ALREADY EXISTS", item);
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.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode192
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:192: /// project file
full stop at end of comment ( here and other places)
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode197
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:197: var buildDirectory = TemplateDirectory + @"\Build";
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode204
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:204: var projectFile = Directory.GetFiles(buildDirectory, "*.csproj").FirstOrDefault();
Why firstorDefault here, if no project do we not want to throw?
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode205
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:205: TraceSource.TraceEvent(TraceEventType.Information, "{0}\t Building \"{1}\"", item, projectFile);
if projectFile == null
throw new exception ?
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode244
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:244: TraceSource.TraceEvent(TraceEventType.Information, "{0}\t Pushing to NuGet repository", item);
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode262
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:262: private string CreateLocalNupkgFile(string buildDirectory)
DirectoryInfo
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode279
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs:279: TraceSource.TraceEvent(TraceEventType.Information, "{0}\t {1} file was created", item, nuspec);
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs (right):
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode73
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:73: const string DownloadBundleTempDirectory = @"C:\Temp\NuGetPublisher\";
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode75
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:75: /// <summary>The download URI format to download a specific API format</summary>
Document the number of parameters and what you hope to be put in.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode77
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";
line length
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode77
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";
Optional: @ is not required for this string.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode86
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:86: static Program()
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode89
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"
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode99
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:99: {
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))
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode103
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:103: // TODO(peleyal): add explanation which option is missing
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode166
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:166: TraceSource.TraceEvent(TraceEventType.Information, "\"{0}\" folder was created", DownloadBundleTempDirectory);
line length
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode193
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:193: await item.Run();
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode215
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs:215: await item.Test(@"Z:\Shared\NuGet\");
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.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs
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.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs#newcode11
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs:11: [assembly: AssemblyCompany("")]
Do you want to put in google here and trademark?
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs#newcode35
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs:35: [assembly: AssemblyVersion("1.0.0.0")]
suggestion: I like having at least one * in the version number.
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs (right):
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs#newcode16
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs:16:
Out of time, have not reviewed.
Message from unknown
2013-08-14T20:39:16+00:00peleyalurn:md5:4ed9aec828567cc4ea750a813b570aef
Message from unknown
2013-08-14T21:04:58+00:00peleyalurn:md5:7d7ba1293848421a5597718c15c20198
Message from peleyal@google.com
2013-08-14T21:05:24+00:00peleyalurn:md5:8cf975075f7b7575a7fc73808a31c350
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 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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs (right):
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode33
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode39
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode41
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode45
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode45
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode54
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode54
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode57
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode58
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode61
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode70
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode72
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode73
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode74
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode83
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode98
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode124
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode124
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode143
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/v1beta1/csharp
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode144
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode144
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode157
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode168
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode170
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode173
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode192
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode197
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode204
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode205
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode244
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode262
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode279
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs (right):
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode73
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode75
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode77
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode77
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode86
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode89
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode99
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode103
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode166
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode193
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode215
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.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs
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.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs#newcode11
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.Publisher/Google.Apis.NuGet.Publisher/Properties/AssemblyInfo.cs#newcode35
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.Publisher/Google.Apis.NuGet.Publisher/Utils.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs (right):
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs#newcode16
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. :)
Message from unknown
2013-08-15T20:19:39+00:00peleyalurn:md5:fb015800bfed28a636c0058032f134e3
Message from davidwaters@google.com
2013-08-16T10:18:36+00:00David watersurn:md5:48488c6d3d4d4a0a29d645622c5001c7
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs (right):
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode33
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode41
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode45
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode72
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode279
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode54
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode139
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.Publisher/Google.Apis.NuGet.Publisher/Utils.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs#newcode25
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.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
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.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs#newcode20
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.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryService.cs
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.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryService.cs#newcode27
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.Publisher/Google.Apis.NuGet.Publisher/packages.config
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config#newcode3
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.Publisher/Template/.nuget/NuGet.Config
File Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config#newcode8
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
?
Message from unknown
2013-08-16T13:42:34+00:00peleyalurn:md5:9f15bf6c9f1e98bf3528d7fda34a3783
Message from unknown
2013-08-16T13:51:47+00:00peleyalurn:md5:ce2224608772c1066e85cfb2414db5da
Message from unknown
2013-08-16T13:56:36+00:00peleyalurn:md5:d45a8aa2022b359b8c239eaea315e2c7
Message from peleyal@google.com
2013-08-16T13:57:43+00:00peleyalurn:md5:5fe6a8bd1ba79a845343fb54049c37f1
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs (right):
https://codereview.appspot.com/12662047/diff/13001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode33
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode41
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode45
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode72
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode279
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode54
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.Publisher/Google.Apis.NuGet.Publisher/Program.cs#newcode139
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.Publisher/Google.Apis.NuGet.Publisher/Utils.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utils.cs#newcode25
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.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
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.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs#newcode20
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.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryService.cs
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.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryService.cs#newcode27
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.Publisher/Google.Apis.NuGet.Publisher/packages.config
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config#newcode3
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.Publisher/Template/.nuget/NuGet.Config
File Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Template/.nuget/NuGet.Config#newcode8
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.
Message from unknown
2013-08-16T21:53:44+00:00peleyalurn:md5:9189e6a98d453a92b221058fe657e3f6
Message from unknown
2013-08-19T20:52:46+00:00peleyalurn:md5:b6e61e0b856c915ea4505e69c94694b8
Message from unknown
2013-08-19T21:19:56+00:00peleyalurn:md5:7f4848f327c73a6759f4f271c04cf74a
Message from davidwaters@google.com
2013-08-19T22:16:36+00:00David watersurn:md5:2b7bcabada0f86d7a2acb9e9f5d8f531
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 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.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs#newcode20
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.Publisher/Google.Apis.NuGet.Publisher/packages.config
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config#newcode3
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?
>
Message from unknown
2013-08-20T16:09:27+00:00peleyalurn:md5:1f2811d472c33ebd7022707ace9562a7
Message from unknown
2013-08-20T16:12:19+00:00peleyalurn:md5:730dd4326c3be694e47eb057f6751ee9
Message from unknown
2013-08-20T16:16:05+00:00peleyalurn:md5:7c003d52c1262ea4ff86642451c8d48f
Message from peleyal@google.com
2013-08-20T16:17:02+00:00peleyalurn:md5:b75d5d4d569642e2be22b4c782f7a76c
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 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.Publisher/Google.Apis.NuGet.Publisher/[Discovery]/DiscoveryDoc.cs#newcode20
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.Publisher/Google.Apis.NuGet.Publisher/packages.config
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config (right):
https://codereview.appspot.com/12662047/diff/48001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/packages.config#newcode3
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?
> >
>
Message from unknown
2013-08-21T14:44:42+00:00peleyalurn:md5:88059a098e4e2ffd24dbc2cf2404004c
Message from davidwaters@google.com
2013-08-27T18:20:11+00:00David watersurn:md5:0d6591a60da6f869f0933ae4e6ac5673
LGTM
A few more comments but I will leave it to you to chose to implement or not.
David.
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs (right):
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode35
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>
s/package/packages
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode35
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>
s/A nuget publisher which publishes/Publishes
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs (right):
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs#newcode40
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs:40: // if the destination directory doesn't exist, create it.
Do you want to ClearOrCreateDirectory the Destination directory first?
https://codereview.appspot.com/12662047/diff/137001/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/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Discovery/DiscoveryDoc.cs#newcode20
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Discovery/DiscoveryDoc.cs:20: public class DiscoveryDoc
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.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs (right):
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs#newcode31
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs:31: static readonly TraceSource TraceSource = new TraceSource("Google.Apis");
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.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs#newcode31
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs:31: static readonly TraceSource TraceSource = new TraceSource("Google.Apis");
private?
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs#newcode34
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs:34: public const string LocalNuGetPackageFolder = @"C:\LocalNuGetFeed";
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.
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs#newcode79
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs:79: PackageServer ps = new PackageServer("https://nuget.org/", "Google.Apis");
longer variable name please. (aberrations discouraged)
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs (right):
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs#newcode25
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs:25: public static class Utilities
Utils.Utilities is a pretty meaningless name.
TraceExtensions?
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs#newcode25
Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs:25: public static class Utilities
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).
Message from unknown
2013-08-27T19:51:42+00:00peleyalurn:md5:7babd873f9e085f8df870dfca8303dd2
Message from peleyal@google.com
2013-08-27T19:54:59+00:00peleyalurn:md5:8fe0bd1a31a9b8c5d0a074f5bf0a54f6
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs (right):
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode35
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.Publisher/Google.Apis.NuGet.Publisher/ApiItemLogic.cs#newcode35
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.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs (right):
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/DirectoryUtilities.cs#newcode40
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.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/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Discovery/DiscoveryDoc.cs#newcode20
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.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs (right):
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs#newcode31
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.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs#newcode31
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.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs#newcode34
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.Publisher/Google.Apis.NuGet.Publisher/NuGetUtilities.cs#newcode79
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.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs
File Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs (right):
https://codereview.appspot.com/12662047/diff/137001/Tools/Google.Apis.NuGet.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs#newcode25
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.Publisher/Google.Apis.NuGet.Publisher/Utilities.cs#newcode25
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.