Skip to content

Commit 68f0638

Browse files
dsplaisteddsplaisted
committed
Apply code review feedback
1 parent 56cc63a commit 68f0638

File tree

7 files changed

+187
-202
lines changed

7 files changed

+187
-202
lines changed

‎src/Cli/dotnet/Commands/Tool/Execute/ToolExecuteCommand.cs‎

Lines changed: 118 additions & 116 deletions
Original file line numberDiff line numberDiff line change
@@ -21,147 +21,149 @@
2121
using NuGet.Versioning;
2222

2323

24-
namespace Microsoft.DotNet.Cli.Commands.Tool.Execute
24+
namespace Microsoft.DotNet.Cli.Commands.Tool.Execute;
25+
26+
internal class ToolExecuteCommand(ParseResult result, ToolManifestFinder? toolManifestFinder = null, string? currentWorkingDirectory = null) : CommandBase(result)
2527
{
26-
internal class ToolExecuteCommand(ParseResult result, ToolManifestFinder? toolManifestFinder = null, string? currentWorkingDirectory = null) : CommandBase(result)
28+
const int ERROR_CANCELLED = 1223; // Windows error code for "Operation canceled by user"
29+
30+
private readonly PackageIdentity _packageToolIdentityArgument = result.GetRequiredValue(ToolExecuteCommandParser.PackageIdentityArgument);
31+
private readonly IEnumerable<string> _forwardArguments = result.GetValue(ToolExecuteCommandParser.CommandArgument) ?? Enumerable.Empty<string>();
32+
private readonly bool _allowRollForward = result.GetValue(ToolExecuteCommandParser.RollForwardOption);
33+
private readonly string? _configFile = result.GetValue(ToolExecuteCommandParser.ConfigOption);
34+
private readonly string[] _sources = result.GetValue(ToolExecuteCommandParser.SourceOption) ?? [];
35+
private readonly string[] _addSource = result.GetValue(ToolExecuteCommandParser.AddSourceOption) ?? [];
36+
private readonly bool _ignoreFailedSources = result.GetValue(ToolCommandRestorePassThroughOptions.IgnoreFailedSourcesOption);
37+
private readonly bool _interactive = result.GetValue(ToolExecuteCommandParser.InteractiveOption);
38+
private readonly VerbosityOptions _verbosity = result.GetValue(ToolExecuteCommandParser.VerbosityOption);
39+
private readonly bool _yes = result.GetValue(ToolExecuteCommandParser.YesOption);
40+
41+
// TODO: Does result.OptionValuesToBeForwarded work here?
42+
private readonly IToolPackageDownloader _toolPackageDownloader = ToolPackageFactory.CreateToolPackageStoresAndDownloader(
43+
additionalRestoreArguments: result.OptionValuesToBeForwarded(ToolExecuteCommandParser.GetCommand())).downloader;
44+
45+
// TODO: Make sure to add these options to the command
46+
private readonly RestoreActionConfig _restoreActionConfig = new RestoreActionConfig(DisableParallel: result.GetValue(ToolCommandRestorePassThroughOptions.DisableParallelOption),
47+
NoCache: result.GetValue(ToolCommandRestorePassThroughOptions.NoCacheOption) || result.GetValue(ToolCommandRestorePassThroughOptions.NoHttpCacheOption),
48+
IgnoreFailedSources: result.GetValue(ToolCommandRestorePassThroughOptions.IgnoreFailedSourcesOption),
49+
Interactive: result.GetValue(ToolCommandRestorePassThroughOptions.InteractiveRestoreOption));
50+
51+
private readonly ToolManifestFinder _toolManifestFinder = toolManifestFinder ?? new ToolManifestFinder(new DirectoryPath(currentWorkingDirectory ?? Directory.GetCurrentDirectory()));
52+
53+
public override int Execute()
2754
{
28-
private readonly PackageIdentity _packageToolIdentityArgument = result.GetRequiredValue(ToolExecuteCommandParser.PackageIdentityArgument);
29-
private readonly IEnumerable<string> _forwardArguments = result.GetValue(ToolExecuteCommandParser.CommandArgument) ?? Enumerable.Empty<string>();
30-
private readonly bool _allowRollForward = result.GetValue(ToolExecuteCommandParser.RollForwardOption);
31-
private readonly string? _configFile = result.GetValue(ToolExecuteCommandParser.ConfigOption);
32-
private readonly string[] _sources = result.GetValue(ToolExecuteCommandParser.SourceOption) ?? [];
33-
private readonly string[] _addSource = result.GetValue(ToolExecuteCommandParser.AddSourceOption) ?? [];
34-
private readonly bool _ignoreFailedSources = result.GetValue(ToolCommandRestorePassThroughOptions.IgnoreFailedSourcesOption);
35-
private readonly bool _interactive = result.GetValue(ToolExecuteCommandParser.InteractiveOption);
36-
private readonly VerbosityOptions _verbosity = result.GetValue(ToolExecuteCommandParser.VerbosityOption);
37-
private readonly bool _yes = result.GetValue(ToolExecuteCommandParser.YesOption);
38-
39-
// TODO: Does result.OptionValuesToBeForwarded work here?
40-
private readonly IToolPackageDownloader _toolPackageDownloader = ToolPackageFactory.CreateToolPackageStoresAndDownloader(
41-
additionalRestoreArguments: result.OptionValuesToBeForwarded(ToolExecuteCommandParser.GetCommand())).Item3;
42-
43-
// TODO: Make sure to add these options to the command
44-
private readonly RestoreActionConfig _restoreActionConfig = new RestoreActionConfig(DisableParallel: result.GetValue(ToolCommandRestorePassThroughOptions.DisableParallelOption),
45-
NoCache: result.GetValue(ToolCommandRestorePassThroughOptions.NoCacheOption) || result.GetValue(ToolCommandRestorePassThroughOptions.NoHttpCacheOption),
46-
IgnoreFailedSources: result.GetValue(ToolCommandRestorePassThroughOptions.IgnoreFailedSourcesOption),
47-
Interactive: result.GetValue(ToolCommandRestorePassThroughOptions.InteractiveRestoreOption));
48-
49-
private readonly ToolManifestFinder _toolManifestFinder = toolManifestFinder ?? new ToolManifestFinder(new DirectoryPath(currentWorkingDirectory ?? Directory.GetCurrentDirectory()));
50-
51-
public override int Execute()
55+
VersionRange versionRange = _parseResult.GetVersionRange();
56+
PackageId packageId = new PackageId(_packageToolIdentityArgument.Id);
57+
58+
// Look in local tools manifest first, but only if version is not specified
59+
if (versionRange == null)
5260
{
53-
VersionRange versionRange = _parseResult.GetVersionRange();
54-
PackageId packageId = new PackageId(_packageToolIdentityArgument.Id);
61+
var localToolsResolverCache = new LocalToolsResolverCache();
5562

56-
// Look in local tools manifest first, but only if version is not specified
57-
if (versionRange == null)
63+
if (_toolManifestFinder.TryFindPackageId(packageId, out var toolManifestPackage))
5864
{
59-
var localToolsResolverCache = new LocalToolsResolverCache();
60-
61-
if (_toolManifestFinder.TryFindPackageId(packageId, out var toolManifestPackage))
65+
var toolPackageRestorer = new ToolPackageRestorer(
66+
_toolPackageDownloader,
67+
_sources,
68+
overrideSources: [],
69+
_verbosity,
70+
_restoreActionConfig,
71+
localToolsResolverCache,
72+
new FileSystemWrapper());
73+
74+
var restoreResult = toolPackageRestorer.InstallPackage(toolManifestPackage, _configFile == null ? null : new FilePath(_configFile));
75+
76+
if (!restoreResult.IsSuccess)
6277
{
63-
var toolPackageRestorer = new ToolPackageRestorer(
64-
_toolPackageDownloader,
65-
_sources,
66-
overrideSources: [],
67-
_verbosity,
68-
_restoreActionConfig,
69-
localToolsResolverCache,
70-
new FileSystemWrapper());
71-
72-
var restoreResult = toolPackageRestorer.InstallPackage(toolManifestPackage, _configFile == null ? null : new FilePath(_configFile));
73-
74-
if (!restoreResult.IsSuccess)
75-
{
76-
Reporter.Error.WriteLine(restoreResult.Message.Red());
77-
return 1;
78-
}
79-
80-
var localToolsCommandResolver = new LocalToolsCommandResolver(
81-
_toolManifestFinder,
82-
localToolsResolverCache);
83-
84-
return ToolRunCommand.ExecuteCommand(localToolsCommandResolver, toolManifestPackage.CommandNames.Single().Value, _forwardArguments, _allowRollForward);
78+
Reporter.Error.WriteLine(restoreResult.Message.Red());
79+
return 1;
8580
}
81+
82+
var localToolsCommandResolver = new LocalToolsCommandResolver(
83+
_toolManifestFinder,
84+
localToolsResolverCache);
85+
86+
return ToolRunCommand.ExecuteCommand(localToolsCommandResolver, toolManifestPackage.CommandNames.Single().Value, _forwardArguments, _allowRollForward);
8687
}
88+
}
8789

88-
var packageLocation = new PackageLocation(
89-
nugetConfig: _configFile != null ? new(_configFile) : null,
90-
sourceFeedOverrides: _sources,
91-
additionalFeeds: _addSource);
90+
var packageLocation = new PackageLocation(
91+
nugetConfig: _configFile != null ? new(_configFile) : null,
92+
sourceFeedOverrides: _sources,
93+
additionalFeeds: _addSource);
9294

93-
var restoreActionConfig = new RestoreActionConfig(
94-
IgnoreFailedSources: _ignoreFailedSources,
95-
Interactive: _interactive);
95+
var restoreActionConfig = new RestoreActionConfig(
96+
IgnoreFailedSources: _ignoreFailedSources,
97+
Interactive: _interactive);
9698

97-
(var bestVersion, var packageSource) = _toolPackageDownloader.GetNuGetVersion(packageLocation, packageId, _verbosity, versionRange, restoreActionConfig);
99+
(var bestVersion, var packageSource) = _toolPackageDownloader.GetNuGetVersion(packageLocation, packageId, _verbosity, versionRange, restoreActionConfig);
98100

99-
IToolPackage toolPackage;
101+
IToolPackage toolPackage;
100102

101-
// TODO: Add framework argument
102-
if (!_toolPackageDownloader.TryGetDownloadedTool(packageId, bestVersion, targetFramework: null, out toolPackage))
103+
// TODO: Add framework argument
104+
if (!_toolPackageDownloader.TryGetDownloadedTool(packageId, bestVersion, targetFramework: null, out toolPackage))
105+
{
106+
if (!UserAgreedToRunFromSource(packageId, bestVersion, packageSource))
103107
{
104-
if (!UserAgreedToRunFromSource(packageId, bestVersion, packageSource))
108+
if (_interactive)
105109
{
106-
if (_interactive)
107-
{
108-
// TODO: Should we return an exit code that indicates the user canceled the operation?
109-
throw new GracefulException(CliCommandStrings.ToolDownloadCanceled, isUserError: true);
110-
}
111-
else
112-
{
113-
throw new GracefulException(CliCommandStrings.ToolDownloadNeedsConfirmation, isUserError: true);
114-
}
110+
Reporter.Error.WriteLine(CliCommandStrings.ToolDownloadCanceled.Red().Bold());
111+
return ERROR_CANCELLED;
112+
}
113+
else
114+
{
115+
Reporter.Error.WriteLine(CliCommandStrings.ToolDownloadNeedsConfirmation.Red().Bold());
116+
return 1;
115117
}
116-
117-
// We've already determined which source we will use and displayed that in a confirmation message to the user.
118-
// So set the package location here to override the source feeds to just the source we already resolved to.
119-
// This does mean that we won't work with feeds that have a primary package but where the RID-specific packages are on
120-
// other feeds, but this is probably OK.
121-
var downloadPackageLocation = new PackageLocation(
122-
nugetConfig: _configFile != null ? new(_configFile) : null,
123-
sourceFeedOverrides: [packageSource.Source],
124-
additionalFeeds: _addSource);
125-
126-
toolPackage = _toolPackageDownloader.InstallPackage(
127-
downloadPackageLocation,
128-
packageId: packageId,
129-
verbosity: _verbosity,
130-
versionRange: new VersionRange(bestVersion, true, bestVersion, true),
131-
isGlobalToolRollForward: false,
132-
restoreActionConfig: restoreActionConfig);
133118
}
134119

135-
var commandSpec = ToolCommandSpecCreator.CreateToolCommandSpec(toolPackage.Command.Name.Value, toolPackage.Command.Executable.Value, toolPackage.Command.Runner, _allowRollForward, _forwardArguments);
136-
var command = CommandFactoryUsingResolver.Create(commandSpec);
137-
var result = command.Execute();
138-
return result.ExitCode;
120+
// We've already determined which source we will use and displayed that in a confirmation message to the user.
121+
// So set the package location here to override the source feeds to just the source we already resolved to.
122+
// This does mean that we won't work with feeds that have a primary package but where the RID-specific packages are on
123+
// other feeds, but this is probably OK.
124+
var downloadPackageLocation = new PackageLocation(
125+
nugetConfig: _configFile != null ? new(_configFile) : null,
126+
sourceFeedOverrides: [packageSource.Source],
127+
additionalFeeds: _addSource);
128+
129+
toolPackage = _toolPackageDownloader.InstallPackage(
130+
downloadPackageLocation,
131+
packageId: packageId,
132+
verbosity: _verbosity,
133+
versionRange: new VersionRange(bestVersion, true, bestVersion, true),
134+
isGlobalToolRollForward: false,
135+
restoreActionConfig: restoreActionConfig);
139136
}
140137

141-
private bool UserAgreedToRunFromSource(PackageId packageId, NuGetVersion version, PackageSource source)
138+
var commandSpec = ToolCommandSpecCreator.CreateToolCommandSpec(toolPackage.Command.Name.Value, toolPackage.Command.Executable.Value, toolPackage.Command.Runner, _allowRollForward, _forwardArguments);
139+
var command = CommandFactoryUsingResolver.Create(commandSpec);
140+
var result = command.Execute();
141+
return result.ExitCode;
142+
}
143+
144+
private bool UserAgreedToRunFromSource(PackageId packageId, NuGetVersion version, PackageSource source)
145+
{
146+
if (_yes)
142147
{
143-
if (_yes)
144-
{
145-
return true;
146-
}
148+
return true;
149+
}
147150

148-
if (!_interactive)
149-
{
150-
return false;
151-
}
151+
if (!_interactive)
152+
{
153+
return false;
154+
}
152155

153-
// TODO: Use a better way to ask for user input
154-
// TODO: How to localize y/n and interpret keys correctly? Does Spectre.Console handle this?
155-
string promptMessage = string.Format(CliCommandStrings.ToolDownloadConfirmationPrompt + " ", packageId, version.ToString(), source.Source);
156+
// TODO: Use a better way to ask for user input
157+
// TODO: How to localize y/n and interpret keys correctly? Does Spectre.Console handle this?
158+
string promptMessage = string.Format(CliCommandStrings.ToolDownloadConfirmationPrompt + " ", packageId, version.ToString(), source.Source);
156159

157-
Console.Write(promptMessage);
158-
bool userAccepted = Console.ReadKey().Key == ConsoleKey.Y;
160+
Console.Write(promptMessage);
161+
bool userAccepted = Console.ReadKey().Key == ConsoleKey.Y;
159162

160-
Console.WriteLine();
161-
// TODO: Do we want a separator like this? Seems like it's meant to separate the output of the tool from the prompt.
162-
//Console.WriteLine(new String('-', promptMessage.Length + 2));
163+
Console.WriteLine();
164+
// TODO: Do we want a separator like this? Seems like it's meant to separate the output of the tool from the prompt.
165+
//Console.WriteLine(new String('-', promptMessage.Length + 2));
163166

164-
return userAccepted;
165-
}
167+
return userAccepted;
166168
}
167169
}

0 commit comments

Comments
 (0)