From 6ac69811e2031e7f9a20416041ae2dd17b726b4b Mon Sep 17 00:00:00 2001 From: Noah Gilson Date: Mon, 12 May 2025 10:29:33 -0700 Subject: [PATCH 1/7] Set the default value of Create Manifest If Needed to True This should enable the findFirst function to create a tool manifest by default. Will break some tests. On initial glance,, the arg behavior only seems to be inherited by tool install local, which is all I want to impact right now. Although it also gets inherited by the construct command in command parser. --- .../dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs b/src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs index da55163d75c0..a4d0b7b86254 100644 --- a/src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs +++ b/src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs @@ -51,7 +51,8 @@ internal static class ToolInstallCommandParser public static readonly Option CreateManifestIfNeededOption = new("--create-manifest-if-needed") { Description = CliCommandStrings.CreateManifestIfNeededOptionDescription, - Arity = ArgumentArity.Zero + Arity = ArgumentArity.Zero, + DefaultValueFactory = _ => true, }; public static readonly Option AllowPackageDowngradeOption = new("--allow-downgrade") From 6c059f994755a727e35329d32d77c1a54753b2c8 Mon Sep 17 00:00:00 2001 From: Noah Gilson Date: Mon, 12 May 2025 11:16:29 -0700 Subject: [PATCH 2/7] Add test to show it uses manifest if needed by default --- .../Install/ToolInstallLocalCommandTests.cs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs b/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs index e1871f2ca072..4b4e3442591f 100644 --- a/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs +++ b/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs @@ -408,6 +408,29 @@ public void GivenNoManifestFileAndCreateManifestIfNeededFlagItShouldCreateManife _fileSystem.File.Exists(Path.Combine(_temporaryDirectory, ".config", "dotnet-tools.json")).Should().BeTrue(); } + [Fact] + public void GivenNoManifestFileItUsesCreateManifestIfNeededByDefault() + { + _fileSystem.File.Delete(_manifestFilePath); + + ParseResult parseResult = + Parser.Instance.Parse( + $"dotnet tool install {_packageIdA.ToString()}"); + + + var installLocalCommand = new ToolInstallLocalCommand( + parseResult, + _packageIdA, + _toolPackageDownloaderMock, + _toolManifestFinder, + _toolManifestEditor, + _localToolsResolverCache, + _reporter); + + installLocalCommand.Execute().Should().Be(0); + _fileSystem.File.Exists(Path.Combine(_temporaryDirectory, ".config", "dotnet-tools.json")).Should().BeTrue(); + } + [Fact] public void GivenNoManifestFileAndCreateManifestIfNeededFlagItShouldCreateManifestInSln() { From 9973ff09fdda79d8992023cbf4abb42ddbca3de4 Mon Sep 17 00:00:00 2001 From: Noah Gilson Date: Mon, 12 May 2025 11:22:26 -0700 Subject: [PATCH 3/7] Update Tests - We dont need two tests showing it should throw, and then it should throw with a message. Kept only the 2nd test. - Modified the we should throw test to only expect throw if create manifest if needed is false since the default is now true. --- .../Install/ToolInstallLocalCommandTests.cs | 29 +++++++++---------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs b/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs index 4b4e3442591f..b6bc62224cdd 100644 --- a/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs +++ b/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs @@ -127,23 +127,22 @@ public void WhenRunWithPackageIdItShouldSaveToCacheAndAddToManifestFile() AssertDefaultInstallSuccess(); } - [Fact] - public void GivenNoManifestFileItShouldThrow() - { - _fileSystem.File.Delete(_manifestFilePath); - var toolInstallLocalCommand = GetDefaultTestToolInstallLocalCommand(); - - Action a = () => toolInstallLocalCommand.Execute(); - a.Should().Throw() - .And.Message.Should() - .Contain(CliStrings.CannotFindAManifestFile); - } - [Fact] public void GivenNoManifestFileItShouldThrowAndContainNoManifestGuide() { _fileSystem.File.Delete(_manifestFilePath); - var toolInstallLocalCommand = GetDefaultTestToolInstallLocalCommand(); + ParseResult parseResult = + Parser.Instance.Parse( + $"dotnet tool install {_packageIdA.ToString()} --create-manifest-if-needed=false"); + + var toolInstallLocalCommand = new ToolInstallLocalCommand( + parseResult, + _packageIdA, + _toolPackageDownloaderMock, + _toolManifestFinder, + _toolManifestEditor, + _localToolsResolverCache, + _reporter); Action a = () => toolInstallLocalCommand.Execute(); a.Should().Throw() @@ -417,11 +416,9 @@ public void GivenNoManifestFileItUsesCreateManifestIfNeededByDefault() Parser.Instance.Parse( $"dotnet tool install {_packageIdA.ToString()}"); - var installLocalCommand = new ToolInstallLocalCommand( parseResult, - _packageIdA, - _toolPackageDownloaderMock, + _toolPackageInstallerMock, _toolManifestFinder, _toolManifestEditor, _localToolsResolverCache, From ac1eae1659f9e8f68ed53bb34f1859549a506f74 Mon Sep 17 00:00:00 2001 From: Noah Gilson Date: Mon, 12 May 2025 11:35:20 -0700 Subject: [PATCH 4/7] Fix Test --- .../CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs b/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs index b6bc62224cdd..d51b12b06b93 100644 --- a/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs +++ b/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs @@ -418,7 +418,8 @@ public void GivenNoManifestFileItUsesCreateManifestIfNeededByDefault() var installLocalCommand = new ToolInstallLocalCommand( parseResult, - _toolPackageInstallerMock, + _packageIdA, + _toolPackageDownloaderMock, _toolManifestFinder, _toolManifestEditor, _localToolsResolverCache, From 11d59e6329ddca137313e01e02a66aebdde773cc Mon Sep 17 00:00:00 2001 From: Noah Gilson Date: Mon, 12 May 2025 13:11:36 -0700 Subject: [PATCH 5/7] Accept 'false' instead of just empty arity and add 0 arity test --- .../Tool/Install/ToolInstallCommandParser.cs | 2 +- .../Install/ToolInstallLocalCommandTests.cs | 22 ++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs b/src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs index a4d0b7b86254..8296baa02ba7 100644 --- a/src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs +++ b/src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs @@ -51,7 +51,7 @@ internal static class ToolInstallCommandParser public static readonly Option CreateManifestIfNeededOption = new("--create-manifest-if-needed") { Description = CliCommandStrings.CreateManifestIfNeededOptionDescription, - Arity = ArgumentArity.Zero, + Arity = ArgumentArity.ZeroOrOne, DefaultValueFactory = _ => true, }; diff --git a/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs b/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs index d51b12b06b93..b1d51d58f0ec 100644 --- a/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs +++ b/test/dotnet.Tests/CommandTests/Tool/Install/ToolInstallLocalCommandTests.cs @@ -127,13 +127,33 @@ public void WhenRunWithPackageIdItShouldSaveToCacheAndAddToManifestFile() AssertDefaultInstallSuccess(); } + [Fact] + public void GivenCreateManifestIfNeededWithoutArgumentTheDefaultIsTrueForLegacyBehavior() + { + _fileSystem.File.Delete(_manifestFilePath); + ParseResult parseResult = + Parser.Instance.Parse( + $"dotnet tool install {_packageIdA.ToString()} --create-manifest-if-needed"); + + var toolInstallLocalCommand = new ToolInstallLocalCommand( + parseResult, + _packageIdA, + _toolPackageDownloaderMock, + _toolManifestFinder, + _toolManifestEditor, + _localToolsResolverCache, + _reporter); + + toolInstallLocalCommand.Execute().Should().Be(0); + } + [Fact] public void GivenNoManifestFileItShouldThrowAndContainNoManifestGuide() { _fileSystem.File.Delete(_manifestFilePath); ParseResult parseResult = Parser.Instance.Parse( - $"dotnet tool install {_packageIdA.ToString()} --create-manifest-if-needed=false"); + $"dotnet tool install {_packageIdA.ToString()} --create-manifest-if-needed false"); var toolInstallLocalCommand = new ToolInstallLocalCommand( parseResult, From ff21731fbda4fa57e091c911cc4b1c4c15deb082 Mon Sep 17 00:00:00 2001 From: Noah Gilson Date: Mon, 12 May 2025 14:43:34 -0700 Subject: [PATCH 6/7] Update completions test bash --- .../bash/DotnetCliSnapshotTests.VerifyCompletions.verified.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/dotnet.Tests/CompletionTests/snapshots/bash/DotnetCliSnapshotTests.VerifyCompletions.verified.sh b/test/dotnet.Tests/CompletionTests/snapshots/bash/DotnetCliSnapshotTests.VerifyCompletions.verified.sh index 6e76bbce5854..6a7eeb7f841d 100644 --- a/test/dotnet.Tests/CompletionTests/snapshots/bash/DotnetCliSnapshotTests.VerifyCompletions.verified.sh +++ b/test/dotnet.Tests/CompletionTests/snapshots/bash/DotnetCliSnapshotTests.VerifyCompletions.verified.sh @@ -1647,6 +1647,10 @@ _testhost_tool_install() { COMPREPLY=( $(compgen -W "d detailed diag diagnostic m minimal n normal q quiet" -- "$cur") ) return ;; + --create-manifest-if-needed) + COMPREPLY=( $(compgen -W "False True" -- "$cur") ) + return + ;; esac COMPREPLY=( $(compgen -W "$opts" -- "$cur") ) From 99b0d0e81731e542037967ddfd3aa293d500ea56 Mon Sep 17 00:00:00 2001 From: Noah Gilson Date: Mon, 12 May 2025 15:02:37 -0700 Subject: [PATCH 7/7] Update snapshots 1. Run Test 2. dotnet restore .\dotnet.Tests\ /t:CompareCliSnapshots in test folder on test prj 3. dotnet restore .\dotnet.Tests\ /t:UpdateCliSnapshots --- .../DotnetCliSnapshotTests.VerifyCompletions.verified.nu | 2 +- .../zsh/DotnetCliSnapshotTests.VerifyCompletions.verified.zsh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/dotnet.Tests/CompletionTests/snapshots/nushell/DotnetCliSnapshotTests.VerifyCompletions.verified.nu b/test/dotnet.Tests/CompletionTests/snapshots/nushell/DotnetCliSnapshotTests.VerifyCompletions.verified.nu index bd3568c9fe1e..4024e6b4d046 100644 --- a/test/dotnet.Tests/CompletionTests/snapshots/nushell/DotnetCliSnapshotTests.VerifyCompletions.verified.nu +++ b/test/dotnet.Tests/CompletionTests/snapshots/nushell/DotnetCliSnapshotTests.VerifyCompletions.verified.nu @@ -22,4 +22,4 @@ let-env config = { completer: $external_completer # add it here } } -} +} \ No newline at end of file diff --git a/test/dotnet.Tests/CompletionTests/snapshots/zsh/DotnetCliSnapshotTests.VerifyCompletions.verified.zsh b/test/dotnet.Tests/CompletionTests/snapshots/zsh/DotnetCliSnapshotTests.VerifyCompletions.verified.zsh index 109551b4e1a4..d9b0a60082e8 100644 --- a/test/dotnet.Tests/CompletionTests/snapshots/zsh/DotnetCliSnapshotTests.VerifyCompletions.verified.zsh +++ b/test/dotnet.Tests/CompletionTests/snapshots/zsh/DotnetCliSnapshotTests.VerifyCompletions.verified.zsh @@ -1044,7 +1044,7 @@ _testhost() { '-v=[Set the MSBuild verbosity level. Allowed values are q\[uiet\], m\[inimal\], n\[ormal\], d\[etailed\], and diag\[nostic\].]:LEVEL:((d\:"d" detailed\:"detailed" diag\:"diag" diagnostic\:"diagnostic" m\:"m" minimal\:"minimal" n\:"n" normal\:"normal" q\:"q" quiet\:"quiet" ))' \ '--arch=[The target architecture.]: : ' \ '-a=[The target architecture.]: : ' \ - '--create-manifest-if-needed[Create a tool manifest if one isn'\''t found during tool installation. For information on how manifests are located, see https\://aka.ms/dotnet/tools/create-manifest-if-needed]' \ + '--create-manifest-if-needed=[Create a tool manifest if one isn'\''t found during tool installation. For information on how manifests are located, see https\://aka.ms/dotnet/tools/create-manifest-if-needed]: :((False\:"False" True\:"True" ))' \ '--allow-downgrade[Allow package downgrade when installing a .NET tool package.]' \ '--allow-roll-forward[Allow a .NET tool to roll forward to newer versions of the .NET runtime if the runtime it targets isn'\''t installed.]' \ '--help[Show command line help.]' \