CLI Utils: file-scoped namespaces and house-keeping #47430
Conversation
… (non extension) classes to use file-scoped namespaces. Updated exclusion.dic with some more domain-specific words.
# Conflicts: # src/Cli/Microsoft.DotNet.Cli.Utils/EnvironmentProvider.cs # src/Cli/Microsoft.DotNet.Cli.Utils/GracefulException.cs # src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs # src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs # src/Cli/dotnet/CommandFactory/CommandResolution/PackagedCommandSpecFactory.cs # src/Cli/dotnet/commands/dotnet-test/MSBuildUtility.cs
There was a problem hiding this comment.
PR Overview
This PR cleans up the CLI codespace by standardizing namespace declarations to file-scoped syntax and performing general code refactoring.
- Converts multi-line namespace declarations into file-scoped namespaces in multiple files
- Introduces minor refactors such as static method conversion in Command.cs and lambda expression usage in ArgumentEscaper.cs
- Updates the DotnetFiles.cs property initialization style
Reviewed Changes
| File | Description |
|---|---|
| DotnetVersionFile.cs, DotDefaultPathCorrector.cs, DependencyProvider.cs, Command.cs, DebugHelper.cs, CommandUnknownException.cs, AnsiConsole.cs, ExponentialRetry.cs, EnvironmentProvider.cs, CommandResult.cs, CommandAvailableAsDotNetToolException.cs, CommandLoggingContext.cs, Constants.cs | Converted to file-scoped namespaces and removed extraneous closing braces |
| Env.cs | Refactored to use expression-bodied members with a readonly environment provider |
| BlockingMemoryStream.cs | Changed the way the BlockingCollection is instantiated |
| ArgumentEscaper.cs | Refactored multi-line method definitions into expression-bodied members |
Copilot reviewed 93 out of 93 changed files in this pull request and generated 1 comment.
| public int ExitCode { get; } | ||
| public string? StdOut { get; } | ||
| public string? StdErr { get; } | ||
| public readonly struct CommandResult(ProcessStartInfo startInfo, int exitCode, string? stdOut, string? stdErr) |
There was a problem hiding this comment.
Do we like primary constructors? 🤔
There was a problem hiding this comment.
I wasn't aware of it, but I like it
| @@ -1,7 +1,9 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
|
|
|||
| #if NETCOREAPP | |||
There was a problem hiding this comment.
I'd personally rather let usings slide even if they aren't necessary as long as they aren't harmful for this sort of case
There was a problem hiding this comment.
There doesn't seem to be consistency as some files explicitly include usings within a #if directive (likely necessary) while others don't. I'd rather be explicit because to the IDE, this statement appears greyed out unless you have the correct directives defined based on the configuration view you're using. It is a little bit more "correct" this way.
| @@ -196,7 +196,7 @@ private Task<IResponse> OnRequest(IRequest request) | |||
|
|
|||
| default: | |||
| // If it doesn't match any of the above, throw an exception | |||
| throw new NotSupportedException(string.Format(LocalizableStrings.CmdUnsupportedMessageRequestTypeException, request.GetType())); | |||
| throw new NotSupportedException(string.Format(Microsoft.DotNet.Tools.Test.LocalizableStrings.CmdUnsupportedMessageRequestTypeException, request.GetType())); | |||
There was a problem hiding this comment.
Is this level of specificity really needed? Seemed like there are several instances late in this PR
There was a problem hiding this comment.
LocalizableStrings exists in multiple namespaces. The change for some of the files to use the correct Utils namespace made a conflict exist. This resolves the conflict.
In this specific case, Microsoft.DotNet.Tools.Common did not have LocalizableStrings but Microsoft.DotNet.Cli.Utils does. That's why there is a conflict now as that class also exists in Microsoft.DotNet.Tools.Test. The only other way is to create a using alias for this version of that class and use that alias here.
Tip
Hide whitespace changesto review this PR.Summary
I'm continuing doing general cleanup in the CLI codespace.
Changes
FileAccessRetrier.csto match class name