Use Microsoft.Extensions.CommandLineUtils and Microsoft.Extensions.Tools.Internal in service ref programs #13323

dougbu · 2019-08-22T01:32:54Z

add '--quiet' option
add '--debug' option (when building Debug configuration)
support -- in dotnet-getdocument; users may want to explictly add options for GetDocument.Insider
remove '--no-color' option
use IReporter extension methods instead of static Reporter class
reduce static use to ease testing (coming soon) and share the IConsole and IReporter
- add ProgramBase
allow mix of known and unknown command-line arguments
- maintain existing behaviour using switch added in Add switch to enable expected and unexpected arguments in any order extensions#2210

nits:

add a couple more CommandLineApplicationExtensions methods
take VS suggestions

dougbu · 2019-08-22T01:36:34Z

I'll get dotnet/extensions#2210 in tomorrow morning (giving reviewers a bit more time) and this won't build until dependencies flow to this repo. But, I wanted to give reviewers more time with this.

Reviewers feel free to add others familiar with our command-line tools if I missed anyone. @pranavkm I know you're probably the most familiar w/ the service ref code 🥇

ryanbrandenburg

This area seems to be without tests, that's tracked here but we should be careful that that doesn't slip out beyond like the 3.1 milestone.

I'll leave final approval for whoever has been doing your other approvals on this work.

dougbu · 2019-08-23T03:59:10Z

@pranavkm @rynowak please do not ignore this PR. Need to get it in tomorrow.

…ols.Internal in service ref programs - add '--quiet' option - add '--debug' option (when building Debug configuration) - support `--` in `dotnet-getdocument`; users may want to explictly add options for GetDocument.Insider - remove '--no-color' option - use `IReporter` extension methods instead of static `Reporter` class - reduce `static` use to ease testing (coming soon) and share the `IConsole` and `IReporter` - add `ProgramBase` - allow mix of known and unknown command-line arguments - maintain existing behaviour using switch added in dotnet/extensions#2210 nits: - add a couple more `CommandLineApplicationExtensions` methods - take VS suggestions

NTaylorMullen

If everyone else is good with getting this in then it's fine with me 😉

NTaylorMullen · 2019-08-23T18:12:26Z

src/Tools/GetDocumentInsider/src/ProgramBase.cs

+        public ProgramBase(IConsole console)
+        {
+            _console = console ?? throw new ArgumentNullException(nameof(console));
+            _reporter = new ConsoleReporter(_console, verbose: false, quiet: false);


Does it make more sense to treat quiet/verbose similar to debug and pre-parse those arguments -> Create a reporter at the top level -> flow to any command interested? I ask because you create a reporter here in ProgramBase, the CommandBase constructor and in the CommandBase configure. Seems like a whole lot of maintenance for a reporter.

--debug is "special" because it's supported only as the first command-line argument, not understood by any CommandOption, and generally hidden. The others are real options that can appear anywhere in the command line. Besides that, this is the pattern we've used elsewhere.

BTW the truly-extra Reporter instances are created for Exception handling in Program classes whether or not the command line has been parsed. The --quiet and --verbose options don't matter when writing errors.

NTaylorMullen · 2019-08-23T18:16:05Z

src/Tools/GetDocumentInsider/src/ProgramBase.cs

+            }
+            catch (Exception ex)
+            {
+                if (ex is CommandException || ex is CommandParsingException)


Just translate this into a catch -> when for clarity.

Will file a follow-up issue for this and replacing ReporterExtensions

NTaylorMullen · 2019-08-23T18:17:54Z

src/Tools/GetDocumentInsider/src/ProgramBase.cs

+                    AllowArgumentSeparator = !throwOnUnexpectedArg,
+                    Error = _console.Error,
+                    FullName = Resources.CommandFullName,
+                    Name = Resources.CommandFullName,


Should these really be resources? If the command name gets localized doesn't that lead to misinformation on how to invoke the command?

Eventually, perhaps. However most of our tools are not yet localizable and I'd rather stick w/ the norm until / unless we decide to change that all up.

NTaylorMullen · 2019-08-23T18:19:54Z

src/Tools/GetDocumentInsider/src/ReporterExtensions.cs

+
+namespace Microsoft.Extensions.ApiDescription.Tool
+{
+    internal static class ReporterExtensions


Why not just write your own type that inherits from ConsoleReporter? Seems sketchy to have a global static to control prefixing the output.

There is one but it was specific to dotnet-watch until @ryanbrandenburg checked in the OpenAPI global tool (CLI for adding references that use the Microsoft.Extensions.ApiDescription.Client infrastructure). So, the timing didn't work out but we could reconsider in 3.1.

Will file a follow-up issue for this and your catch clarity suggestion.

NTaylorMullen · 2019-08-23T18:22:02Z

src/Tools/Shared/CommandLine/CommandLineApplicationExtensions.cs

@@ -21,6 +21,17 @@ public static void OnExecute(this CommandLineApplication app, Action action)
                    return 0;
                });

+        public static CommandOption Option(this CommandLineApplication command, string template, string description)


Is this needed? I don't see it used anywhere. Also seems a little too magical for a general use-case extension

Yes it's needed; none of the Option(...) calls in these tools pass a CommandOptionType.

NTaylorMullen · 2019-08-23T18:25:35Z

src/Tools/dotnet-getdocument/src/Exe.cs

@@ -32,100 +34,30 @@ internal static class Exe
                startInfo.WorkingDirectory = workingDirectory;
            }

-            using (var process = Process.Start(startInfo))
+            using var process = Process.Start(startInfo);


Why change to use this variant of using? Having the block level using makes it clear when you expect the process has exited.

Compiler goodies. Remove the bracket will help in defining the terse syntax for using scope and that it is pinned to function.

BrennanConroy · 2019-08-23T18:34:49Z

src/Tools/Shared/CommandLine/CommandLineApplicationExtensions.cs

+                template,
+                description,
+                template.IndexOf('<') != -1
+                    ? template.EndsWith(">...") ? CommandOptionType.MultipleValue : CommandOptionType.SingleValue


This looks odd, could you explain?

This is a copy / paste from https://github.com/aspnet/AspNetCore/pull/13323/files#diff-e22cb33793ce6707b3111cac914249dfL8 (the old CommandLineApplicationExtensions used in these tools). The idea is to infer the CommandOptionType from the template. It's a compact version of the template parsing CommandLineApplication does when trying to determine the name of an option's value. The three cases are

no '<' indicates the option has no value

if there is a '<', ">..." indicates the option accepts multiple values

otherwise, the option accepts a single value

Yes, the code ignores the possibility of '<' without a matching '>' but the CommandLineApplication template will throw in that case IIRC. If not, it's a general bug in the Extensions repo.

dougbu · 2019-08-23T22:54:46Z

Getting this in. The only failure was of a test that's marked flaky but seems to be running everywhere.

dougbu added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-code-generation tell-mode Indicates a PR which is being merged during tell-mode labels Aug 22, 2019

dougbu requested review from pranavkm, rynowak, BrennanConroy and ryanbrandenburg August 22, 2019 01:32

dougbu force-pushed the dougbu/service.ref.todos.4923.II branch from f5458a9 to f2520e8 Compare August 22, 2019 01:33

ryanbrandenburg reviewed Aug 22, 2019

View reviewed changes

dougbu mentioned this pull request Aug 23, 2019

Clean up TODOs and duplicated code in client code generation #4923

Closed

3 tasks

dougbu force-pushed the dougbu/service.ref.todos.4923.II branch from f2520e8 to 1d512c4 Compare August 23, 2019 16:02

NTaylorMullen approved these changes Aug 23, 2019

View reviewed changes

BrennanConroy reviewed Aug 23, 2019

View reviewed changes

BrennanConroy approved these changes Aug 23, 2019

View reviewed changes

dougbu merged commit e89a051 into release/3.0 Aug 23, 2019

ghost deleted the dougbu/service.ref.todos.4923.II branch August 23, 2019 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Microsoft.Extensions.CommandLineUtils and Microsoft.Extensions.Tools.Internal in service ref programs #13323

Use Microsoft.Extensions.CommandLineUtils and Microsoft.Extensions.Tools.Internal in service ref programs #13323

dougbu commented Aug 22, 2019 •

edited

Loading

dougbu commented Aug 22, 2019

ryanbrandenburg left a comment

dougbu commented Aug 23, 2019

NTaylorMullen left a comment

NTaylorMullen Aug 23, 2019

dougbu Aug 23, 2019

NTaylorMullen Aug 23, 2019

dougbu Aug 23, 2019

NTaylorMullen Aug 23, 2019

dougbu Aug 23, 2019

NTaylorMullen Aug 23, 2019

dougbu Aug 23, 2019

NTaylorMullen Aug 23, 2019

dougbu Aug 23, 2019

NTaylorMullen Aug 23, 2019

dcarr42 Aug 23, 2019 •

edited

Loading

BrennanConroy Aug 23, 2019

dougbu Aug 23, 2019

dougbu commented Aug 23, 2019

Use Microsoft.Extensions.CommandLineUtils and Microsoft.Extensions.Tools.Internal in service ref programs #13323

Use Microsoft.Extensions.CommandLineUtils and Microsoft.Extensions.Tools.Internal in service ref programs #13323

Conversation

dougbu commented Aug 22, 2019 • edited Loading

dougbu commented Aug 22, 2019

ryanbrandenburg left a comment

Choose a reason for hiding this comment

dougbu commented Aug 23, 2019

NTaylorMullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcarr42 Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Aug 23, 2019

dougbu commented Aug 22, 2019 •

edited

Loading

dcarr42 Aug 23, 2019 •

edited

Loading