Add support for '--' argument separator #159

natemcmaster · 2016-09-06T22:28:19Z

~~Depends on dotnet/extensions#145.~~ ✅ merged

Lays the foundation for allowing dotnet-watch to have command line flags such as --verbose.

Update README.md.

victorhurdugaci · 2016-09-06T23:04:12Z

src/Microsoft.DotNet.Watcher.Tools/Program.cs

+            var options = CommandLineOptions.Parse(args, _out);
+            if (options == null)
+            {
+                _error.WriteLine("Unexpected error parsing command line arguments");


Does it print details? If not, how do I know the error so I can fix it?

This is a "should never happen" condition...maybe I should use Debug.Assert instead...?

victorhurdugaci · 2016-09-06T23:05:36Z

Is there any plan to add any arguments right now? If not, I'm not sure that we want this change yet /cc @glennc

natemcmaster · 2016-09-06T23:06:50Z

I was thinking of adding --log <LEVEL> and/or --verbose

victorhurdugaci · 2016-09-06T23:08:39Z

Sync with @glennc , I think he should approve that. Right now you have way to pass it. I think if the number of options we plan to add to dotnet watch is small (<5), we shouldn't complicate the command line, especially if those options are used by very few people.

natemcmaster · 2016-09-08T20:48:43Z

Most usages don't need '--'. It's an optional arg that makes definitive which args are "mine" vs "theirs".

IMO it's not a matter of complicating/simplifying the command line. This is about env variables. Long environment var names are hard to discover and clunky to set. Adding '--' would allows us to do command line args for these settings (e.g. --log or --polling).

cc @glennc

natemcmaster · 2016-09-13T19:20:56Z

Talked with @glennc. The clarify this, the primary purpose of '--' is to make deterministic the split between which args belong to dotnet-watch (subset 1) and which should be passed through to the inside command (subset 2).

The only good example we have of this at the moment is '--help'

Input	How dotnet-watch splits args into the sub command
`dotnet watch --help`	`dotnet watch --help` / `(no sub args)`
`dotnet watch -- --help`	`dotnet watch` / `--help`

But let's say we start adding more options, like --verbose, here are implications:

Input	How dotnet-watch splits args into the sub command
`dotnet watch --verbose -- run --verbose -- a b c`	`dotnet watch --verbose` / `run --verbose -- a b c`
`dotnet watch -- run --verbose -- a b c`	`dotnet watch --verbose` / `run --verbose -- a b c`
`dotnet watch run --verbose -- a b c`	`dotnet watch` / `run --verbose -- a b c`
`dotnet watch --verbose run`	`dotnet watch --verbose` / `run`
`dotnet watch -- --verbose run`	`dotnet watch` / `--verbose run`
`dotnet watch --verbose --verbose run`	Error!
`dotnet watch --verbose -- --verbose run`	`dotnet watch --verbose` / `--verbose run`

campersau · 2016-09-15T12:39:50Z

README.md

 ### How To Install

 Add `Microsoft.DotNet.Watcher.Tools` to the `tools` section of your `project.json` file:

-```
+```js


The json highlighter doesn't parse the '...' well.

natemcmaster · 2016-09-20T21:33:30Z

Ping @glennc. Are you okay with "--" support in syntax for dotnet-watch?

natemcmaster · 2016-09-23T23:51:27Z

Talked with @glennc. We're okay moving ahead with this.
cc @moozzyk

moozzyk

Looks mostly good - just a few minor issues.

moozzyk · 2016-09-24T00:23:31Z

test/Microsoft.DotNet.Watcher.Tools.Tests/CommandLineOptionsTests.cs

+        [InlineData(new object[] { new string[0] })]
+        public void HelpArgs(string[] args)
+        {
+            // arrange


nit: I am not sure what is the style in this repo but I personally don't find these comments useful.

moozzyk · 2016-09-24T00:27:56Z

src/Microsoft.DotNet.Watcher.Tools/Program.cs

+                    .WatchAsync(projectToWatch, options.RemainingArguments, _cancellationToken)
+                    .Wait();
+            }
+            catch (AggregateException ex) when (ex.InnerExceptions.Count == 1 && ex.InnerException is TaskCanceledException)


you could make MainInternal async and then catch only TaskCanceledException. You would .Wait() in Main instead of here.

moozzyk · 2016-09-24T00:30:36Z

src/Microsoft.DotNet.Watcher.Tools/CommandLineOptions.cs

+            var app = new CommandLineApplication(throwOnUnexpectedArg: false)
+            {
+                Name = "dotnet watch",
+                FullName = "Microsoft DotNet File Watcher",


Is this correct casing (DotNet)?

Yes. It looks a little weird, but it's what the DotNet team uses.

moozzyk · 2016-09-24T00:38:10Z

src/Microsoft.DotNet.Watcher.Tools/CommandLineOptions.cs

+                throw new ArgumentNullException(nameof(args));
+            }
+
+            var app = new CommandLineApplication(throwOnUnexpectedArg: false)


what happens in an unexpected argument appears before --? (e.g. dotnet watch --help --bogus)

As per https://github.com/dotnet/cli/blob/rel/1.0.0/src/dotnet/CommandLine/CommandLineApplication.cs#L16-L18 I guess --bogus will be added to RemainingArgs and then we will just exit In Program l. 82

--bogus will be added to RemainingArgs

Yes, that's what will happen. The heuristic for RemainingArgs is: RemainingArgs = all args starting with the first unrecognized arg or the first arg after the '--'

moozzyk · 2016-09-24T00:46:12Z

test/Microsoft.DotNet.Watcher.Tools.Tests/CommandLineOptionsTests.cs

+        [Theory]
+        [InlineData(new object[] { new[] { "-h" } })]
+        [InlineData(new object[] { new[] { "-?" } })]
+        [InlineData(new object[] { new[] { "--help" } })]


add a case where there is an argument after --help

moozzyk · 2016-09-24T00:49:43Z

src/Microsoft.DotNet.Watcher.Tools/Program.cs

 using Microsoft.Extensions.Logging;

 namespace Microsoft.DotNet.Watcher.Tools
 {
    public class Program
    {
        private readonly ILoggerFactory _loggerFactory;
+        private readonly CancellationToken _cancellationToken;
+        private readonly TextWriter _error;


Is this being used at all?

Not at the moment. Will remove.

moozzyk · 2016-09-24T00:52:21Z

src/Microsoft.DotNet.Watcher.Tools/CommandLineOptions.cs

+{
+    internal class CommandLineOptions
+    {
+        public bool IsHelp { get; set; }


This should not be settable outside this class.

moozzyk · 2016-09-24T00:53:28Z

src/Microsoft.DotNet.Watcher.Tools/CommandLineOptions.cs

+    internal class CommandLineOptions
+    {
+        public bool IsHelp { get; set; }
+        public IList<string> RemainingArguments { get; set; }


Not that this matter as much (you can always change contents) but it should not be settable outside this class.

actually both could have just getters...

IReadOnlyList with a private setter?

I would stick just to IList<string> with a private setter. No reason to make it more complicated than this...

moozzyk · 2016-09-24T00:54:51Z

src/Microsoft.DotNet.Watcher.Tools/CommandLineOptions.cs

+
+            app.HelpOption("-?|-h|--help");
+
+            var options = new CommandLineOptions();


nit: move down where you actually need it.

natemcmaster · 2016-09-26T16:52:43Z

🆙 📅

moozzyk · 2016-09-26T17:03:47Z

src/Microsoft.DotNet.Watcher.Tools/Program.cs


-        public Program()
+        public Program(CancellationToken cancellationToken)


Is this ctor needed at all? Why not make the other one public and remove this one? We don't seem to use it (unless there are some tests using it)..

Yeah, can be removed. Opened this PR so long ago I can't remember what I was thinking when I added the public/internal ctor split.

moozzyk · 2016-09-26T17:04:09Z

One small comment and then 🚢🇮🇹

Also refactors command line parsing into a separate class.

dnfclas added the cla-already-signed label Sep 6, 2016

natemcmaster force-pushed the namc/arguments branch 2 times, most recently from 7c24650 to 3c816f3 Compare September 6, 2016 22:56

victorhurdugaci reviewed Sep 6, 2016
View reviewed changes

natemcmaster mentioned this pull request Sep 8, 2016

Add support for argument separator '--' dotnet/extensions#145

Merged

natemcmaster force-pushed the namc/arguments branch from 3c816f3 to 3333b1c Compare September 13, 2016 23:25

campersau reviewed Sep 15, 2016

View reviewed changes

natemcmaster force-pushed the namc/arguments branch 2 times, most recently from 8c17c45 to 1bd19dc Compare September 20, 2016 00:26

moozzyk reviewed Sep 24, 2016

View reviewed changes

moozzyk reviewed Sep 26, 2016

View reviewed changes

moozzyk approved these changes Sep 26, 2016

View reviewed changes

natemcmaster force-pushed the namc/arguments branch from dc7900a to 9026b61 Compare September 26, 2016 17:15

Add support for '--' argument separator

f90594a

Also refactors command line parsing into a separate class.

natemcmaster force-pushed the namc/arguments branch from 9026b61 to f90594a Compare September 26, 2016 17:20

natemcmaster merged commit f90594a into dev Sep 26, 2016

natemcmaster deleted the namc/arguments branch September 26, 2016 17:26


		app.HelpOption("-?\|-h\|--help");

		var options = new CommandLineOptions();


		public Program()
		public Program(CancellationToken cancellationToken)

Add support for '--' argument separator #159

Add support for '--' argument separator #159

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victorhurdugaci commented Sep 6, 2016

Uh oh!

natemcmaster commented Sep 6, 2016

Uh oh!

victorhurdugaci commented Sep 6, 2016

Uh oh!

natemcmaster commented Sep 8, 2016

Uh oh!

natemcmaster commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natemcmaster commented Sep 20, 2016

Uh oh!

natemcmaster commented Sep 23, 2016

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

moozzyk Sep 24, 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!

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 commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moozzyk commented Sep 26, 2016

Uh oh!

Uh oh!

natemcmaster commented Sep 6, 2016 •

edited

Loading

moozzyk Sep 24, 2016 •

edited

Loading