refactor: More directory layout refactoring #14007
Conversation
| @@ -383,7 +382,7 @@ private int ExecuteForUnconfigured(string[] args) | |||
| private void GenerateUnconfigured(string arg) | |||
| { | |||
| Console.WriteLine($"{DateTime.UtcNow:yyyy-MM-dd'T'HH:mm:ss.fff}Z Generating {arg}"); | |||
| var apiDirectory = Path.Combine(_rootLayout.GeneratorOutput, arg); | |||
There was a problem hiding this comment.
This is deliberately a change btw - it was broken before. (It should have been relative to _rootLayout.Googleapis.)
There was a problem hiding this comment.
Approving but there's a bug on one of the snippets source properties, I've flagged it there.
Otherwise, the only thing I'm not certain about is the sepration in two of ApiLayout. I understand the need to use one or the other of repo-root and output, but other than that I don't understand why we'd need the distinction. And the using of one root or the other can be solved with factory methods. But, I know I might be missing something, so this is not a blocker for me.
|
|
||
| // Type just to allow us to write a constructor that is obviously geared towards help snippets, | ||
| // and ensure that the calling code is clear too. | ||
| private sealed class HelpSnippetsMarker { } |
There was a problem hiding this comment.
nit: Or magic word like "help" received as id?
There was a problem hiding this comment.
"help" is actually used below for deciding on which factory method to call.
Although I see it goes throug ApiLayou which would have to understand "help" as well.
There was a problem hiding this comment.
Yes, I've basically been trying to localize magic, as it were - being explicit in code where possible.
| /// <summary> | ||
| /// Directory for snippets for the library, including hand-written snippets. | ||
| /// </summary> | ||
| public string SnippetsDirectory => Path.Combine(SourceDirectory, $"{Id}.GeneratedSnippets"); |
There was a problem hiding this comment.
I can't suggest, but this one shouldn't have the "Generated" part.
| public string GeneratedSnippetsDirectory => Path.Combine(SourceDirectory, $"{Id}.GeneratedSnippets"); | ||
|
|
||
| /// <summary> | ||
| /// Directory for purely generated snippets for the library. |
There was a problem hiding this comment.
nit: Update after copy/paste
| public string UnitTestsDirectory => Path.Combine(SourceDirectory, $"{Id}.Tests"); | ||
|
|
||
| /// <summary> | ||
| /// Directory for purely generated snippets for the library. |
There was a problem hiding this comment.
nit: Update after copy/paste
| /// </summary> | ||
| public class ApiLayout | ||
| public class RepositoryApiLayout |
There was a problem hiding this comment.
Could this one inherit from GeneratorApiLayout so that a lot of the properties don't need to be redefined? both root and output should be the same on the repo, right?
There was a problem hiding this comment.
Could they even go back to being be the same type ApiLayout with two factory methods each choosing the right of ReposirotyRoot and GeneratorOutput?
| var sourceRoot = rootLayout.CreateApiLayout(options.Package).SourceDirectory; | ||
| file = Path.Combine(sourceRoot, options.Package, "bin", "Release", options.Framework, $"{options.Package}.dll"); | ||
| var productionRoot = rootLayout.CreateRepositoryApiLayout(options.Package).ProductionDirectory; | ||
| file = Path.Combine(rootLayout.CreateRepositoryApiLayout(options.Package).GetReleaseAssembly(options.Framework)); |
There was a problem hiding this comment.
I think you don't need Path.Combine anymore.
There was a problem hiding this comment.
Can you create a single RepositoryApiLayout and reuse in both lines?
There was a problem hiding this comment.
Yup, it's much smaller now.
| @@ -564,6 +563,9 @@ bool HasApis(string file) | |||
| } | |||
| } | |||
|
|
|||
| private string GetApiProtosDirectory(ApiMetadata api) => GetApiProtosDirectory(api.ProtoPath); | |||
There was a problem hiding this comment.
ApiLayout? Or in the two that replaced it?
There was a problem hiding this comment.
No, that doesn't know the path to the protos for the API (and can't, if we've just provided an ID).
|
Thanks - will fix the comments in the morning. |
This may change further, but it's a cleaner separation of what most tooling needs vs doc-specific tooling.
jskeet
force-pushed
the
more-directory-layout
branch
2 times, most recently
from
ae60d83 to
a0b0ddf
Compare
Separating the two into different types with different properties makes it less likely that someone will use the wrong one. For example, because |
Building requires a full repo; generation requires only generator output/input directories. This is also the first pass of centralising knowledge of things like "the production source directory". At this point most *directories* are standardised; we could potentially start moving knowledge of filenames (e.g. OwlBot configuratoin etc) - if we see significant benefit.
jskeet
force-pushed
the
more-directory-layout
branch
from
a0b0ddf to
c870250
Compare
No description provided.