Create and use console reporter API #237

natemcmaster · 2016-11-23T20:31:19Z

The reporter API breaks down complex formatter in to composable compontents. Adds 'ReporterBuilder' and 'FormatterBuilder' as syntax sugar for creating reporters with complex formatting rules

Changes:

Remove dependency on Microsoft.Extensions.Logging and instead use IReporter for console output
Only use color output when stdout and stderr are not being redirected
dotnet-watch: Make the default output less noisy

Resolves #227

cc @muratg

natemcmaster · 2016-11-30T18:09:48Z

Should resolve #227.

The formatting:
verbose = dark gray, only appears when --verbose specified
output = normal, white text. Does not appear if --quiet specified.
warn = yellow text
error = red text

dotnet-watch prefixes all its lines with watch : to distinguish from the sub-process's output.

Some examples of output:
dotnet watch run

dotnet watch --verbose run

dotnet user-secrets list --verbose

dotnet user-secret list --verbose with errors

cc @CesarBS

cesarblum · 2016-11-30T19:14:44Z

src/Microsoft.DotNet.Watcher.Tools/Internal/MsBuildFileSetFactory.cs

-                using (var assemblyStream = GetType().GetTypeInfo().Assembly.GetManifestResourceStream("dotnetwatch.targets"))
+                using (
+                    var assemblyStream =
+                        GetType().GetTypeInfo().Assembly.GetManifestResourceStream("dotnetwatch.targets"))


nit: actually prefer the previous format. At least move the var line inline with using.

cesarblum · 2016-11-30T19:43:25Z

src/Microsoft.Extensions.Caching.SqlConfig.Tools/Program.cs

+                .Verbose(f => f.WithColor(ConsoleColor.DarkGray))
+                .Warn(f => f.WithColor(ConsoleColor.Yellow))
+                .Error(f => f.WithColor(ConsoleColor.Red))
+                .Build();


Shouldn't this use the same logic used in CreateReporter in the watcher?

This tool doesn't actually have --verbose flags. Though I suppose detecting output redirection for colorless output is still valid...although probably a very unlikely usage.

cesarblum · 2016-11-30T19:44:25Z

src/Microsoft.Extensions.Caching.SqlConfig.Tools/Program.cs

@@ -86,7 +95,7 @@ public int Run(string[] args)
            }
            catch (Exception exception)
            {
-                _logger.LogCritical("An error occurred. {ErrorMessage}", exception.Message);
+                _reporter.Error("An error occurred. " + exception.Message);


nit: use string interpolation instead of +

cesarblum · 2016-11-30T19:44:53Z

src/Microsoft.Extensions.Caching.SqlConfig.Tools/Program.cs

-                            ex.Message);
+                        _reporter.Error(
+                            $"An error occurred while trying to create the table and index. {ex.Message}"
+                            );


nit: move ); to line above

cesarblum · 2016-11-30T19:51:52Z

src/Microsoft.Extensions.SecretManager.Tools/Program.cs

+                    if (!_console.IsOutputRedirected)
+                    {
+                        f.WithColor(ConsoleColor.DarkGray);
+                    }


What if it is redirected?

It's output is colorless. Users might pipe commands to a file. We want to avoid spitting out the ANSI color codes.

cesarblum · 2016-11-30T19:53:03Z

src/Shared/CliContext.cs

+        public static bool IsGlobalVerbose()
+        {
+            bool globalVerbose;
+            bool.TryParse(Environment.GetEnvironmentVariable("DOTNET_CLI_CONTEXT_VERBOSE"), out globalVerbose);


A lot of environment variables work with true and 1. Shouldn't you check that here? Or do the docs instruct people to only set this to true if they want to enable it?

it's always the result of bool.ToString(). cref https://github.com/dotnet/cli/blob/rel/1.0.0/src/dotnet/Program.cs#L164

cesarblum · 2016-11-30T19:53:54Z

src/Shared/Reporting/ColorFormatter.cs

+
+namespace Microsoft.Extensions.Tools.Internal
+{
+    public class ColorFormatter : IFormatter


Don't we have similar logic in the console logger already?

Yeah. It has an IConsole API internally. The API was insufficient because it didn't provide access to Console.In.

Follow up: we could remove duplication by putting this API and others in a common location to be used by Logging.Console. Could help with aspnet/Logging#491

Yeah it would be good to consolidate these things in a single place. Can you open an issue to track this?

dotnet/extensions#173

cesarblum · 2016-11-30T19:56:53Z

src/Shared/Reporting/FormattingReporter.cs

+        public void Error(string message)
+            => Write(_console.Error, _error.Format(message));
+
+        private void Write(TextWriter writer, string message)


You're not using writer here.

Add tests to verify each of the methods that call this are writing to the expected stream.

Oh wow, yeah, dumb mistake.

cesarblum · 2016-11-30T22:41:04Z

src/Microsoft.Extensions.Caching.SqlConfig.Tools/Program.cs

            try
            {
                var description = "Creates table and indexes in Microsoft SQL Server database " +
-                    "to be used for distributed caching";
+                                  "to be used for distributed caching";


nit: make it a single string.

cesarblum · 2016-11-30T22:43:15Z

Just approved, but: are you adding the tests to make sure each level is writing to the expected stream (out or err)?

natemcmaster · 2016-11-30T22:44:07Z

test/Microsoft.Extensions.Tools.Tests/ReporterTests.cs

+        }
+
+        [Fact]
+        public void WritesToStandardStreams()


@CesarBS Added it :)

when tests pass.

The reporter API breaks down complex formatter in to composable compontents. Adds 'ReporterBuilder' and 'FormatterBuilder' as syntax sugar for creating reporters with complex formatting rules Changes in dotnet-watch - Remove dependency on Microsoft.Extensions.Logging and instead use IReporter for console output - Only use color output when stdout and stderr are not being redirected - Make the default output less noisy Changes in dotnet-user-secrets - Remove dependency on Microsoft.Extensions.Logging to use IReporter Changes in dotnet-sql-cache - Remove dependency on Microsoft.Extensions.Logging to use IReporter - Add --verbose option

dnfclas added the cla-already-signed label Nov 23, 2016

natemcmaster force-pushed the namc/reporter branch 2 times, most recently from 095613b to bb051ed Compare November 29, 2016 20:25

cesarblum suggested changes Nov 30, 2016

View reviewed changes

cesarblum approved these changes Nov 30, 2016

View reviewed changes

natemcmaster commented Nov 30, 2016

View reviewed changes

natemcmaster force-pushed the namc/reporter branch from 7bcad6b to 17da524 Compare November 30, 2016 22:52

natemcmaster merged commit 17da524 into feature/msbuild Dec 1, 2016

natemcmaster deleted the namc/reporter branch December 1, 2016 06:12

This was referenced Dec 1, 2016

Make log format consistent across tools #227

Closed

[RC.2] Cherry-pick commits from feature/msbuild #240

Closed

Create and use console reporter API #237

Create and use console reporter API #237

Uh oh!

Conversation

natemcmaster commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natemcmaster commented Nov 30, 2016

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!

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!

Choose a reason for hiding this comment

Uh oh!

natemcmaster Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

cesarblum commented Nov 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

natemcmaster commented Nov 23, 2016 •

edited

Loading

natemcmaster Nov 30, 2016 •

edited

Loading