sln-add: Refactor logic for autogenerating solution folders #48726
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the logic for autogenerating solution folders in the .NET CLI.
- Shared default arrays for platforms and build types were introduced in SlnFileFactory.
- The SolutionAddCommand was updated to use a member variable for the solution file full path and to streamline project addition.
- New error handling and logic for generating intermediate solution folders based on project paths have been implemented.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Cli/dotnet/SlnFileFactory.cs | Added default arrays for platforms and build types to support solution generation refactor. |
| src/Cli/dotnet/Commands/Solution/Add/SolutionAddCommand.cs | Refactored usage of solution file path and updated project addition and solution folder generation logic. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Adding a couple of additional refactorings to the rest of the command as it too was a bit confusing |
There was a problem hiding this comment.
Pull Request Overview
Refactor the logic for autogenerating solution folders to improve maintainability and readability. Key changes include:
- Extraction of default platform and build type arrays into SlnFileFactory.
- Addition of error handling with a try-catch wrapper in CreateFromFileOrDirectory.
- Introduction of helper methods (e.g. IsSolutionFolderPathInDirectoryScope and GenerateIntermediateSolutionFoldersForProjectPath) to centralize solution folder path handling.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Cli/dotnet/SlnFileFactory.cs | Added try-catch error handling and default arrays for build types/platforms to simplify solution file creation. |
| src/Cli/dotnet/Commands/Solution/Add/SolutionAddCommand.cs | Refactored project addition logic and introduced helper functions to better manage solution folders and project paths. |
| ISolutionSerializer serializer = SolutionSerializers.GetSerializerByMoniker(solutionPath)!; | ||
| return serializer.OpenAsync(solutionPath, CancellationToken.None).Result; | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
The catch block in CreateFromFileOrDirectory rethrows a new GracefulException using only ex.Message, which can lose the original stack trace. Consider including the exception object itself (e.g. passing 'ex' instead of 'ex.Message') to aid in debugging.
Copilot uses AI. Check for mistakes.
| throw new GracefulException(CliStrings.InvalidSolutionFormatString, solutionFileFullPath, ex.Message); | ||
| } | ||
| throw new GracefulException(ex.Message, ex); | ||
| relativeSolutionFolderPath = Path.Combine([.. relativeSolutionFolderPath.Split(Path.DirectorySeparatorChar).SkipLast(1)]); |
There was a problem hiding this comment.
[nitpick] Using a collection expression directly within Path.Combine may reduce code clarity. Consider extracting the computed array into a separate variable before passing it to Path.Combine.
| relativeSolutionFolderPath = Path.Combine([.. relativeSolutionFolderPath.Split(Path.DirectorySeparatorChar).SkipLast(1)]); | |
| var parentDirectories = relativeSolutionFolderPath.Split(Path.DirectorySeparatorChar).SkipLast(1).ToArray(); | |
| relativeSolutionFolderPath = Path.Combine(parentDirectories); |
Copilot uses AI. Check for mistakes.
| { | ||
| string solutionRelativeProjectPath = Path.GetRelativePath(Path.GetDirectoryName(_solutionFileFullPath), fullProjectPath); |
There was a problem hiding this comment.
[nitpick] The variable name 'solutionRelativeProjectPath' is used in multiple scopes with similar contexts. Renaming one occurrence (or scoping it more tightly) may improve code clarity and reduce potential confusion.
| string solutionRelativeProjectPath = Path.GetRelativePath(Path.GetDirectoryName(_solutionFileFullPath), fullProjectPath); | |
| string relativeProjectPath = Path.GetRelativePath(Path.GetDirectoryName(_solutionFileFullPath), fullProjectPath); |
Copilot uses AI. Check for mistakes.
| { | ||
| solution.AddBuildType(buildType); | ||
| } | ||
| SlnFileFactory.DefaultPlatforms.ToList().ForEach(solution.AddPlatform); |
There was a problem hiding this comment.
I think I commented on this at some point, but I prefer the for loops
There was a problem hiding this comment.
Yes you are very right, completely forgot about that one sorry
| } | ||
| catch (SolutionArgumentException ex) when (ex.ParamName == "projectTypeName") | ||
| catch (SolutionArgumentException ex) when (ex.Type == SolutionErrorType.InvalidProjectTypeReference) |
There was a problem hiding this comment.
== is seldom best except for primitives
The logic for handling the autogeneration of solution folders has been refactored to improve the maintainability and readability of the codebase.