refactor: Concentrate knowledge of directory locations #13998

jskeet · 2024-12-13T11:13:27Z

No description provided.

…ayout This removes a lot of scattered knowledge of where apis.json, googleapis etc are - but still scatters a lot of calls to RootLayout.ForCurrentDirectory(). More refactoring coming.

- Removed parameterless ApiCatalog.Load method: we always pass in a RootLayout not - Provide a RootLayout property in CommandBase, to *vastly* reduce the number of calls to RootLayout.ForCurrentDirectory() (This will make it much easier to run commands in a different context if we ever need to.)

jskeet · 2024-12-13T11:20:59Z

Fixes b/383482997

amanda-tarafa

This is great!

All of my comments are about more refactoring on the same line, i.e. moving more paths into RootLayout or ApiLayout. I'm on the fence about some of them as it would require they layout types to know about special folders and files, and even default values for some of those. I think that's fine, and I think having the layout types and only the layout types know everything about folders and files is fine. But I'm also fine with the not knowing about many specific things.

Any changes you want to make, happy to have those in other PRs.

amanda-tarafa · 2024-12-13T17:26:11Z

tools/Google.Cloud.Tools.Common/RootLayout.cs

+    /// </summary>
+    public string RepositoryRoot => FailIfAbsent(_repositoryRoot);
+
+    private static string FailIfAbsent(string value, [CallerMemberName] string caller = null) =>


Oh this is good!

amanda-tarafa · 2024-12-13T17:30:16Z

tools/Google.Cloud.Tools.GenerateDocfxSources/Program.cs

-            var packageSource = Path.Combine(layout.SourceDirectory, api.Id);
+            var rootLayout = RootLayout.ForCurrentDirectory();
+            var apiLayout = rootLayout.CreateApiLayout(api);
+            var packageSource = Path.Combine(apiLayout.SourceDirectory, api.Id);


Can't this one be brought into ApiLayout? It already knows the api.Id, right?

Yes. (Or at least, it does at build time.) I think we've got this in multiple places, in fact - certainly in generation. Will do as a follow-up. I'll try finding every occurrence of Path.Combine to see whether there are other similar patterns, too.

amanda-tarafa · 2024-12-13T17:36:13Z

tools/Google.Cloud.Tools.GenerateSnippetMarkdown/Program.cs


-            DirectoryLayout layout = api == "help" ? DirectoryLayout.ForHelpSnippets() : DirectoryLayout.ForApi(api);
+            var rootLayout = RootLayout.ForCurrentDirectory();
+            var apiLayout = api == "help" ? ApiLayout.ForHelpSnippets(rootLayout) : rootLayout.CreateApiLayout(api);


Move the question to RootLayout? I'm on the fence about this one btw.

I strongly suspect this is the only occurrence of this - we may want to move the logic into this file, if we can.

amanda-tarafa · 2024-12-13T17:47:35Z

tools/Google.Cloud.Tools.ReleaseManager/ContainerCommands/CleanCommand.cs

                    DeleteGeneratedSource(".Snippets");
                    DeleteGeneratedSource(".GeneratedSnippets");
-                    var generatedSnippetsDirectory = Path.Combine(layout.SourceDirectory, $"{api.Id}.GeneratedSnippets");
+                    var generatedSnippetsDirectory = Path.Combine(apiLayout.SourceDirectory, $"{api.Id}.GeneratedSnippets");


Move to ApiLayout?

Hmm... will have a think. (Again, I'll see how many places we use Path.Combine for this.)

amanda-tarafa · 2024-12-13T17:50:23Z

tools/Google.Cloud.Tools.ReleaseManager/ContainerCommands/CleanCommand.cs

                        DeleteAll(Directory.EnumerateFiles(generatedSnippetsDirectory, "*.json"));
                    }
-                    File.Delete(Path.Combine(layout.SourceDirectory, "gapic_metadata.json"));
+                    File.Delete(Path.Combine(apiLayout.SourceDirectory, "gapic_metadata.json"));


Move as well? Maybe with a ApiLayout.CombineWithSourceDirectory(string fileName) if you prefer not know about specific files in ApiLayout?

Ooh, a method like that would be nice, yes.

amanda-tarafa · 2024-12-13T17:54:30Z

tools/Google.Cloud.Tools.ReleaseManager/ContainerCommands/CleanCommand.cs

+                var directory = Path.Combine(apiLayout.SourceDirectory, $"{api.Id}{directorySuffix}");
                if (Directory.Exists(directory))
                {
                    DeleteAll(Directory.EnumerateFiles(directory, "*.g.cs", SearchOption.AllDirectories));


How about ApiLayout knows about the suffixes, etc. and just gives you the collection of generated files?

Unsure at the moment. Bear in mind that quite a bit of that is gapic-specific.

amanda-tarafa · 2024-12-13T17:58:01Z

tools/Google.Cloud.Tools.ReleaseManager/ContainerCommands/GenerateCommand.cs

-        Environment.SetEnvironmentVariable(GenerateApisCommand.GeneratorInputDirectoryEnvironmentVariable, generatorInput ?? "/app/generator-input");
-        Environment.SetEnvironmentVariable(GenerateApisCommand.GeneratorOutputDirectoryEnvironmentVariable, output);
-        Environment.SetEnvironmentVariable(GenerateApisCommand.GoogleApisDirectoryEnvironmentVariable, apiRoot);
+        var rootLayout = RootLayout.ForGeneration(generatorInput ?? "/app/generator-input", output, apiRoot);


Move this into RootLayout?

I don't think so in this case - it's a really special case of wanting to use /app, which is very specific to the container.

amanda-tarafa · 2024-12-13T18:02:14Z

tools/Google.Cloud.Tools.ReleaseManager/GenerateApisCommand.cs

-        var layout = DirectoryLayout.ForApi(api.Id, generatorOutputDirectory, generatorInputDirectory);
-        string productionDirectory = Path.Combine(layout.SourceDirectory, api.Id);
+        var apiLayout = _rootLayout.CreateApiLayout(api);
+        string productionDirectory = Path.Combine(apiLayout.SourceDirectory, api.Id);


I think this is the same as a built package directory I flagged for moving into ApiLayout a few comments up. If it's here again, maybe it does make sense to move it to ApiLayout.

Yes, exactly what I was thinking :)

amanda-tarafa · 2024-12-13T18:03:35Z

tools/Google.Cloud.Tools.ReleaseManager/GenerateApisCommand.cs

-        var layout = DirectoryLayout.ForApi(api.Id, generatorOutputDirectory, generatorInputDirectory);
-        string productionDirectory = Path.Combine(layout.SourceDirectory, api.Id);
+        var apiLayout = _rootLayout.CreateApiLayout(api);
+        string productionDirectory = Path.Combine(apiLayout.SourceDirectory, api.Id);


And here again, doesn't ApiLayout already knows about the API ID.

jskeet · 2024-12-13T18:19:10Z

Merging, but will do a follow-up on Monday.

jskeet assigned amanda-tarafa Dec 13, 2024

jskeet requested a review from a team as a code owner December 13, 2024 11:13

jskeet added 2 commits December 13, 2024 11:17

refactor: Initial refactoring of DirectoryLayout to RootLayout + ApiL…

306f037

…ayout This removes a lot of scattered knowledge of where apis.json, googleapis etc are - but still scatters a lot of calls to RootLayout.ForCurrentDirectory(). More refactoring coming.

jskeet force-pushed the refactor-directory-layout branch from 7f4b1dd to 439e276 Compare December 13, 2024 11:17

jskeet mentioned this pull request Dec 13, 2024

chore: Remove code coverage, and minor cleanups #13999

Merged

amanda-tarafa approved these changes Dec 13, 2024

View reviewed changes

jskeet merged commit d9d7a4f into googleapis:main Dec 13, 2024
9 checks passed

jskeet deleted the refactor-directory-layout branch December 13, 2024 18:19

refactor: Concentrate knowledge of directory locations #13998

refactor: Concentrate knowledge of directory locations #13998

Uh oh!

Conversation

jskeet commented Dec 13, 2024

Uh oh!

jskeet commented Dec 13, 2024

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jskeet commented Dec 13, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants