Update to System.CommandLine v2

[heaths]

Resolves #559

[heaths]

@AArnott I can keep the argument types, arity, etc. the same, but how much do you care about the help header (nbgv v{version}, etc.) and exact format of the usage? The usage will still be semantically the same, but here's how it looks different so far (IMO, better, but different):

Before

$ nbgv --help
nbgv v3.3.37+0989e8fe0c
Use -h, --help, or -? for usage help. Use after a command to get more help about a particular command.
usage: nbgv <command> [<args>]

    install            Prepares a project to have version stamps applied
                       using Nerdbank.GitVersioning.
    get-version        Gets the version information for a project.
    set-version        Updates the version stamp that is applied to a
                       project.
    tag                Creates a git tag to mark a version.
    get-commits        Gets the commit(s) that match a given version.
    cloud              Communicates with the ambient cloud build to set
                       the build number and/or other cloud build
                       variables.
    prepare-release    Prepares a release by creating a release branch
                       for the current version and adjusting the version
                       on the current branch.

$ nbgv get-version --help
usage: nbgv get-version [-p <arg>] [--metadata <arg>...] [-f <arg>]
            [-v <arg>] [--] <commit-ish>

    -p, --project <arg>     The path to the project or project
                            directory. The default is the current
                            directory.
    --metadata <arg>...     Adds an identifier to the build metadata
                            part of a semantic version.
    -f, --format <arg>      The format to write the version information.
                            Allowed values are: text, json. The default
                            is text.
    -v, --variable <arg>    The name of just one version property to
                            print to stdout. When specified, the output
                            is always in raw text. Useful in scripts.
    <commit-ish>            The commit/ref to get the version
                            information for. The default is HEAD.

After

$ nbgv --help
Usage:
  nbgv [options] [command]

Options:
  --version         Show version information
  -?, -h, --help    Show help and usage information

Commands:
  install                     Prepares a project to have version stamps applied using Nerdbank.GitVersioning.
  get-version <commit-ish>    Gets the version information for a project.
  set-version <version>       Updates the version stamp that is applied to a project.
  tag <versionOrRef>          Creates a git tag to mark a version.
  get-commits <version>       Gets the commit(s) that match a given version.
  cloud                       Communicates with the ambient cloud build to set the build number and/or other cloud build variables.
  prepare-release <tag>       Prepares a release by creating a release branch for the current version and adjusting the version on the current branch.

$ nbgv get-version --help
get-version:
  Gets the version information for a project.

Usage:
  nbgv get-version [options] [<commit-ish>]

Arguments:
  <commit-ish>    The commit/ref to get the version information for. The default is HEAD.

Options:
  -p, --project <project>      The path to the project or project directory. The default is the current directory.
  --metadata <metadata>        Adds an identifier to the build metadata part of a semantic version.
  -f, --format <json|text>     The format to write the version information. Allowed values are: text, json. The default is text.
  -v, --variable <variable>    The name of just one version property to print to stdout. When specified, the output is always in raw text. Useful in scripts.
  -?, -h, --help               Show help and usage information

Also, how much coverage over command line parsing do you roughly estimate you have? From the pipelines running against the PR, it's not readily apparent.

[AArnott]

Thanks. In general the formatting of the usage doc can change. And in particular I like the changes you're showing.

What we must have a serious conversation about is if the actual command syntax changed in any way, because many pipelines just install the latest nbgv tool and run it, so a change in CLI syntax would break pipelines. I didn't see any changes here though. Are you aware of any?

I don't think we have any automated tests for our nbgv CLI parsing.

[AArnott]

This looks good. I see you marked this a WIP. What else needs to be done?

[AArnott]

What do you recommend here? I lean toward keeping this code here because we can ensure an absolute Uri and (I am guessing) print a nicer error message.

[heaths]

We could do both, though maybe not that important. I agree checking if it's an absolute URI is good, but that can also be done at parsing time. Not only can any class with a constructor that takes a sole string be used, but you can have other validation to run at parsing time.

I marked it as a WIP because I didn't want to spend too much time cleaning it up just yet before we discussed it more. This is more POC. Apart from improving parse-time validation (you can also have it accept only legal file paths, or even validate that a file or directory exists at parsing time, not to mention the type-based parsing I mentioned), I also wanted to change the subcommand handlers so I can just call them directly instead of of another anonymous delegate to call them (though, if you don't mind, I could keep them - just adds an extra unnecessary layer).

As for command line handling, this should be compatible. If we're good with proceeding, I'll spend even more time effectively diffing --help output for each subcommand and make sure they line up. I've kept parameter and argument names the same as well.

I know you have quite a bit of "mock repo" tests. It shouldn't be hard to add some tests that do the same thing and execute nbgv commands against it, should it? I could at least add some integration tests for mainline scenarios (maybe even a few corner case scenarios) and exec nbgv before and after my changes. Wouldn't hurt having those tests moving forward either to mitigate regressions.

[AArnott]

I also wanted to change the subcommand handlers so I can just call them directly instead of of another anonymous delegate to call them (though, if you don't mind, I could keep them - just adds an extra unnecessary layer).

That sounds like a good change. I wondered why you had done it that way.

I'll spend even more time effectively diffing --help output for each subcommand and make sure they line up. I've kept parameter and argument names the same as well.

Thanks!

I could at least add some integration tests for mainline scenarios (maybe even a few corner case scenarios) and exec nbgv before and after my changes. Wouldn't hurt having those tests moving forward either to mitigate regressions.

That would be incredible. I wouldn't insist on that to complete the PR, but if you're offering, I'll gratefully accept.

[heaths]

Any particular mainline scenarios you'd like tested? Was thinking:

• get-version
• set-version <version>
• cloud -s GitHubActions -a -c

[AArnott]

Add

• get-version -f json
  and at least coverage of the variants used by the nbgv GitHub Action.

[heaths]

Comparing nbgv v3.3.37+0989e8fe0c to this PR, though all the options were converted from what's currently in master (note: there are existing regressions I've opened separately), the following options appear to be the same except where noted (double-dash handling, though I worked around it):

-nbgv install [-p <arg>] [-v <arg>] [-s <arg>...]
-    -p, --path <arg>                             
-    -v, --version <arg>                          
-    -s, --source <arg>...
+nbgv install [options]
+Options:                  
+  -p, --path <path>       
+  -v, --version <version> 
+  -s, --source <source>   
+  -?, -h, --help          

-usage: nbgv get-version [-p <arg>] [--metadata <arg>...] [-f <arg>]
-            [-v <arg>] [--] <commit-ish>
-    -p, --project <arg> 
-    --metadata <arg>... 
-    -f, --format <arg>  
-    -v, --variable <arg>
-    <commit-ish>        
+nbgv get-version [options] [<commit-ish>]
+Arguments:
+  <commit-ish>    The commit/ref to get the version information for. The default is HEAD.
+Options:                   
+  -p, --project <project>  
+  --metadata <metadata>    
+  -f, --format <json|text>                            
+  -v, --variable <variable>                          
+  -?, -h, --help           

-usage: nbgv set-version [-p <arg>] [--] <version>
-    -p, --project <arg>
-    <version>          
+Usage:
+  nbgv set-version [options] <version>
+Arguments:
+  <version>
+Options:                 
+  -p, --project <project>
+  -?, -h, --help         

With set-version, dotnet exec .\bin\nbgv\Debug\netcoreapp2.1\nbgv.dll set-version -- 10.0.5 yields an error so the double dash is not supported. If important, we may be able to add support for it via middleware.

-usage: nbgv tag [-p <arg>] [--] <versionOrRef>
-    -p, --project <arg>
-    <versionOrRef>     
+Usage:
+  nbgv tag [options] [<versionOrRef>]
+Arguments:      
+  <versionOrRef>
+Options:                 
+  -p, --project <project>
+  -?, -h, --help         

-usage: nbgv get-commits [-p <arg>] [-q] [--] <version>
-    -p, --project <arg>
-    -q, --quiet        
-    <version>          
+Usage:
+  nbgv get-commits [options] <version>
+Arguments: 
+  <version>
+Options:                 
+  -p, --project <project>
+  -q, --quiet            
+  -?, -h, --help         

-usage: nbgv cloud [-p <arg>] [--metadata <arg>...] [-v <arg>] [-s <arg>]
-            [-a] [-c] [-d <arg>...]
-    -p, --project <arg>  
-    --metadata <arg>...  
-    -v, --version <arg>  
-    -s, --ci-system <arg>
-    -a, --all-vars       
-    -c, --common-vars    
-    -d, --define <arg>...
+Usage:
+  nbgv cloud [options]
+Options:                                                  
+  -p, --project <project>                                 
+  --metadata <metadata>                                   
+  -v, --version <version>                                 
+  -s, --ci-system                                         
+  <AppVeyor|AtlassianBamboo|GitHubActions|GitLab|Jenkins|TeamCity|Travis|VisualStudioTeamServices>                
+  -a, --all-vars                                          
+  -c, --common-vars                                       
+  -d, --define <define>                                   
+  -?, -h, --help                                          

-usage: nbgv prepare-release [-p <arg>] [--nextVersion <arg>]
-            [--versionIncrement <arg>] [-f <arg>] [--] <tag>
-    -p, --project <arg>     
-    --nextVersion <arg>     
-    --versionIncrement <arg>
-    -f, --format <arg>      
-    <tag>                   
+Usage:
+  nbgv prepare-release [options] [<tag>]
+Arguments:
+  <tag>   
+Options:                               
+  -p, --project <project>              
+  --nextVersion <nextVersion>          
+  --versionIncrement <versionIncrement>
+  -f, --format <json|text>             
+  -?, -h, --help                       

Though the original usage shows <tag> as required (no square brackets), the parameter description contains "If not specified, any existing prerelease tag will be removed". Thus, it is optional but was not formatted that way. Also, all previous parameter configuration was carried over to the newer version of System.CommandLine so everything should be compatible, sans the double dash before some of the arguments.

[heaths]

I opened dotnet/command-line-api#1238 to track the compatibility issue.

I added some middleware in the meantime to support -- <argument>. It feels a bit hacky but I can't think of any reason anyone would want to use that to pass other unparsed tokens so it's probably safe.

[heaths]

All manual testing we listed worked except for nbgv set-version <version> because of regression #573 that exists in master even without my changes.

[AArnott]

Thank you, @heaths for this contribution. We'll follow up on the other issue(s) you filed.