Updating How Common Props are Generated #109
Conversation
| @@ -0,0 +1,34 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
nit: wrong header #Pending
There was a problem hiding this comment.
on all the new files throughout #Pending
| return Path.Combine(Utilities.AssetPath, $"{unityProjectInfo.UnityProjectName}.{MSBuildFileSuffix}.sln"); | ||
| } | ||
|
|
||
| public void GenerateTopLevelDependenciesProject(UnityProjectInfo unityProjectInfo) |
There was a problem hiding this comment.
GenerateTopLevelDependenciesProject [](start = 20, length = 35)
nit: remove #Pending
| string UnityMajorVersion { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the Unity minir version. |
There was a problem hiding this comment.
minir [](start = 35, length = 5)
nit: minor #Pending
| string UnityMinorVersion { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the current unity platform selected in the editor. |
There was a problem hiding this comment.
unity [](start = 37, length = 5)
nit: Unity should be capitalized #Pending
| /// Writes out the data. | ||
| /// </summary> | ||
| void Write(); | ||
| } |
There was a problem hiding this comment.
This is a somewhat vague function definition. Are multiple files written here? Would it be feasible to return more information on what was written here like a file path? Is it possible that a write could fail and we would want a TryWrite definition here returning a bool to signal success?
|
|
||
| /// <summary> | ||
| /// Generates the Directory.Build.props file that is expected to be used by both generated and non-generated projects alike. | ||
| /// Creates an exporter for the commom MSBuild file that is expected to be used by both generated and non-generated projects alike. |
There was a problem hiding this comment.
commom MSBuild [](start = 40, length = 14)
nit: it might make more sense to actually call out the file name here if its the same across projects
|
|
||
| /// <summary> | ||
| /// Generates the Directory.Build.props file that is expected to be used by both generated and non-generated projects alike. | ||
| /// Creates an exporter for the commom MSBuild file that is expected to be used by both generated and non-generated projects alike. |
There was a problem hiding this comment.
is expected to be used by both generated and non-generated projects alike [](start = 65, length = 73)
nit: I don't know what this description means. I do not know what a generated vs non-generated project is
|
|
||
| protected TemplatedExporterBase(FileTemplate templateFile, string exportPath) | ||
| : base(templateFile.Root, templateFile.Root.CreateReplacementSet()) | ||
| { |
There was a problem hiding this comment.
nit: do we want to check whether templateFile or templateFile.Root is null? Or do we just want the NullPointerException to throw here?
There was a problem hiding this comment.
This is not necessarily public facing code, so no, just throw nullreferenceexception.
| { | ||
| field = value; | ||
| template.Tokens[token].AssignValue(replacementSet, toStringFunc(value)); | ||
| } |
There was a problem hiding this comment.
we should asses whether the token exists in this Tokens collection before calling AssignValue
There was a problem hiding this comment.
Possibly, but since this exporter is tied to the templates, we assume otherwise it's a bug.
There was a problem hiding this comment.
it might be easier to debug a logged error with the token that was missing compared to an exception
In reply to: 368175853 [](ancestors = 368175853)
| /// <param name="dependenciesTargetsTemplatePath">Path to the dependencies targets template file.</param> | ||
| public TemplatedProjectExporter(DirectoryInfo generatedOutputFolder, FileInfo solutionFileTemplatePath, FileInfo projectFileTemplatePath, FileInfo generatedProjectFileTemplatePath, FileInfo projectPropsFileTemplatePath, FileInfo projectTargetsFileTemplatePath, FileInfo msbuildForUnityCommonTemplatePath, FileInfo dependenciesProjectTemplatePath, FileInfo dependenciesPropsTemplatePath, FileInfo dependenciesTargetsTemplatePath) | ||
| public TemplatedUnityProjectExporter(DirectoryInfo generatedOutputFolder, FileInfo solutionFileTemplatePath, FileInfo projectFileTemplatePath, FileInfo generatedProjectFileTemplatePath, FileInfo projectPropsFileTemplatePath, FileInfo projectTargetsFileTemplatePath, FileInfo msbuildForUnityCommonTemplatePath, FileInfo dependenciesProjectTemplatePath, FileInfo dependenciesPropsTemplatePath, FileInfo dependenciesTargetsTemplatePath) | ||
| { |
There was a problem hiding this comment.
nit: this is really hard to read in an editor, I'd prefer if each argument had its own line.
There was a problem hiding this comment.
I may update this, but in a later change.
The purpose of this change is to do a small piecewise update to how we do exporting. This focuses on just the Common props file, but the approach will be added for other parts as well.