[Issue #35173] `--verbosity quiet` suppress success message #36505

Markliniubility · 2023-10-26T23:49:16Z

Before:

> dotnet tool install GitVersion.Tool --global --prerelease --verbosity quiet
Tool 'XXXXXXXXX' (version '999.999.999.999') was successfully installed....

After:

> dotnet tool install GitVersion.Tool --global --prerelease --verbosity quiet
>

I have edited the VerbosityOption as currently the default option is quiet; however, according to the online documentation, the default option should be minimal

I know this it dotnet build but they share the same LoggerVerbosity documentation.

This will affect ~.2% of test work items

…ld 20231024.9 Microsoft.WindowsDesktop.App.Ref , Microsoft.WindowsDesktop.App.Runtime.win-x64 , VS.Redist.Common.WindowsDesktop.SharedFramework.x64.8.0 , VS.Redist.Common.WindowsDesktop.TargetingPack.x64.8.0 From Version 8.0.0-rtm.23524.7 -> To Version 8.0.0

…net#36467) [release/8.0.1xx] Update dependencies from dotnet/windowsdesktop - Coherency Updates: - Microsoft.NET.Sdk.WindowsDesktop: from 8.0.0-rtm.23524.4 to 8.0.0-rtm.23524.6 (parent: Microsoft.WindowsDesktop.App.Ref)

JL03-Yue

Amazing work! Thanks for your contributions!

JL03-Yue · 2023-11-22T20:44:37Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs

+                if (_verbosity.IsQuiet())
+                {
+                    return 0;
+                }
                _reporter.WriteLine(
-                    string.Format(
-                        LocalizableStrings.InstallationSucceeded,
-                        string.Join(", ", package.Commands.Select(c => c.Name)),
-                        package.Id,
-                        package.Version.ToNormalizedString()).Green());
+                string.Format(
+                    LocalizableStrings.InstallationSucceeded,
+                    string.Join(", ", package.Commands.Select(c => c.Name)),
+                    package.Id,
+                    package.Version.ToNormalizedString()).Green());


if !(_verbosity.IsQuiet()) { _reporter.WriteLine( string.Format( LocalizableStrings.InstallationSucceeded, string.Join(", ", package.Commands.Select(c => c.Name)), package.Id, package.Version.ToNormalizedString()).Green()); } return 0;

The affirmative condition might be a better choice. Thanks for pointing it out!

I like the if (!_verbosity.IsQuiet()) option better, too. Nice to have just one exit point.

And also (tiny nit) it'd be nice to have one blank line on either side of the if block.

Forgind

Looks good to me!

Forgind · 2023-11-22T21:07:52Z

src/Cli/dotnet/CommonOptions.cs

-        q,
        minimal,
        m,
+        quiet,


Can you undo this move? They were previously sorted in order of verbosity, and now they aren't sorted at all.

That's a really good point. I was trying to make minimal the default option. Let me try to find other ways to make it default. Thanks!

Forgind · 2023-11-22T21:09:54Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs

+                if (_verbosity.IsQuiet())
+                {
+                    return 0;
+                }
                _reporter.WriteLine(
-                    string.Format(
-                        LocalizableStrings.InstallationSucceeded,
-                        string.Join(", ", package.Commands.Select(c => c.Name)),
-                        package.Id,
-                        package.Version.ToNormalizedString()).Green());
+                string.Format(
+                    LocalizableStrings.InstallationSucceeded,
+                    string.Join(", ", package.Commands.Select(c => c.Name)),
+                    package.Id,
+                    package.Version.ToNormalizedString()).Green());


I like the if (!_verbosity.IsQuiet()) option better, too. Nice to have just one exit point.

Forgind · 2023-11-22T21:11:23Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs

+                if (_verbosity.IsQuiet())
+                {
+                    return 0;
+                }
                _reporter.WriteLine(
-                    string.Format(
-                        LocalizableStrings.InstallationSucceeded,
-                        string.Join(", ", package.Commands.Select(c => c.Name)),
-                        package.Id,
-                        package.Version.ToNormalizedString()).Green());
+                string.Format(
+                    LocalizableStrings.InstallationSucceeded,
+                    string.Join(", ", package.Commands.Select(c => c.Name)),
+                    package.Id,
+                    package.Version.ToNormalizedString()).Green());


And also (tiny nit) it'd be nice to have one blank line on either side of the if block.

Markliniubility · 2023-11-28T11:27:07Z

Looks good to me!

Thank you for your review! Fixed!

Forgind · 2023-11-28T20:54:46Z

Looked at a couple failing legs, and they do seem related to your change. I'm currently leaning towards suggesting that we can just leave the default where it was and have the output change. I think changing the default there changed it for a lot of other commands...

Couple failing tests:
ItCanInvokeDiagramDirective didn't expect verbosity in stdout
ItDoesNotShowImportantLevelMessageByDefault expected quiet verbosity (no "important" text)

Markliniubility · 2023-11-30T21:53:25Z

Looked at a couple failing legs, and they do seem related to your change. I'm currently leaning towards suggesting that we can just leave the default where it was and have the output change. I think changing the default there changed it for a lot of other commands...

Couple failing tests:

ItCanInvokeDiagramDirective didn't expect verbosity in stdout

ItDoesNotShowImportantLevelMessageByDefault expected quiet verbosity (no "important" text)

Thanks for the note, and fixed! Yes that's certainly a very good point. It triggers failures in too many other places: leaving the default as it was is a better idea.

Forgind

I'm happy; thanks for making these changes!

Forgind · 2023-11-30T22:11:24Z

src/Tests/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs

        public void WhenRunWithExactVersionItShouldSucceed()
        {
-            ParseResult result = Parser.Instance.Parse($"dotnet tool install -g {PackageId} --version {PackageVersion}");
+            ParseResult result = Parser.Instance.Parse($"dotnet tool install -g {PackageId} --version {PackageVersion} --verbosity minimal");


Having reverted the default, do you still need to explicitly set verbosity to minimal in all these tests?

Yes. Because by default verbosity is quiet (first element in the verbosity Enum), which has no output unless there is an error.

marcpopMSFT · 2023-12-05T21:47:42Z

Thank you for your contribution!

ghost added Area-Tools untriaged Request triage from a team member labels Oct 26, 2023

Markliniubility changed the title ~~[Issue #35173] --verbosity quiet suppress success message~~ [Issue #35173] WIP: --verbosity quiet suppress success message Oct 27, 2023

Markliniubility marked this pull request as draft October 27, 2023 06:48

Markliniubility and others added 5 commits November 6, 2023 23:00

--verbosity quiet suppress success message

0a95671

Reduce test timeout to speed up timeouts.

a137c83

This will affect ~.2% of test work items

Update src/Tests/UnitTests.proj

1da4ab2

Markliniubility force-pushed the verbosity branch from 1a9f694 to ab89e32 Compare November 7, 2023 07:04

Markliniubility changed the base branch from main to release/8.0.2xx November 7, 2023 07:32

fix verbosity default option

f0edb15

Markliniubility changed the title ~~[Issue #35173] WIP: --verbosity quiet suppress success message~~ [Issue #35173] --verbosity quiet suppress success message Nov 9, 2023

Markliniubility marked this pull request as ready for review November 9, 2023 11:13

Markliniubility mentioned this pull request Nov 9, 2023

dotnet tool has a verbosity mode but it isn't silent in mode "quiet" #35173

Closed

JL03-Yue approved these changes Nov 22, 2023

View reviewed changes

Forgind approved these changes Nov 22, 2023

View reviewed changes

Markliniubility added 2 commits November 28, 2023 03:24

style fix

39b4807

Style Fix

e41e5d4

Remove default verbosity and fix test cases

464619e

Forgind approved these changes Nov 30, 2023

View reviewed changes

marcpopMSFT merged commit b61fe9c into dotnet:release/8.0.2xx Dec 5, 2023

baronfel mentioned this pull request Dec 14, 2023

[NETE2ESDK] [Regression] With .NET 9 SDK installed, it should return that the tool was successfully installed when the tool was installed globally. #37501

Closed

This was referenced Jan 9, 2024

Update dotnet-tool-install.md dotnet/docs#39019

Merged

[9.0.100-alpha.1.24055.6] dotnet tool install command doesn't show install successful message #37879

Closed

[Issue #35173] --verbosity quiet suppress success message #36505

[Issue #35173] --verbosity quiet suppress success message #36505

Uh oh!

Conversation

Markliniubility commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JL03-Yue 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!

Forgind 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!

Markliniubility commented Nov 28, 2023

Uh oh!

Forgind commented Nov 28, 2023

Uh oh!

Markliniubility commented Nov 30, 2023

Uh oh!

Forgind 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!

marcpopMSFT commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Issue #35173] `--verbosity quiet` suppress success message #36505

[Issue #35173] `--verbosity quiet` suppress success message #36505

Markliniubility commented Oct 26, 2023 •

edited

Loading