Command-style cli spike, for #428

[serra]

Work in progress

This is a command-syle cli spike for #428. This breaks the current command line API.

This command(verb)-style cli spike is using gsscoder's commandline parser. Listed documentation (including usage examples) is generated from code (see the unit tests).

It is completely breaking; both from input as output perspective. ATM all argument parsing tests are broken.

Plan:

• Agree on a rough outline of verbs
• Agree on the command line parsing library to use
• how to include (nuget dep or source include)
• Spike a rewrite using a selection of two or three verb
• Implement all the verbs and refine
• Reconsider defaults for input and output
• Verify we have not lost any of the original arguments we originally had

Verbs

First draft, generated from code:

GitVersion
Copyright (c) 2005 - 2015 GitTools

  inspect                 Inspect git repository and output deduced version 
                          information.

  init                    Start configuration utility for gitversion.

  inspect-remote          Inspect a remote repository.

  inject-buildserver      Inject gitversion variables as environment variables 
                          in your build server job.

  inject-msbuild          Build an msbuild file, GitVersion variables will be 
                          injected as msbuild properties.

  inject-process          Executes target executable injecting GitVersion 
                          variables as environmental variables.

  inject-assembly-info    Search for all assembly information files in the git 
                          repo and update them using GitVersion variables.

  help                    Display more information on a specific command.

  version                 Display version information.

verb: inspect

The inspect command will inspect a repository and output GitVersion variables to the command line for user inspection or further processing.

This should probably be the default verb (if such a thing exists).

IMO we should reconsider default output; e.g. simple kv output per line would allow for better output processing by downstream command line tools.

WRT output: IMO we should consider all default console output to be processable by other commandline tools. So no log/debug/warning messages UNLESS there is an exception, the --verbose switch is set or the selected verb+options specify the console output.

USAGE:
Inspect the current directory, output version variables in Json format:
  GitVersion inspect
Inspect different directory:
  GitVersion inspect --path c:\foo\bar\
Show only some variables:
  GitVersion inspect --variables SemVer Major Minor
Output in key=value format, each pair on a new line:
  GitVersion inspect --format KeyValue
Show only one variable's value:
  GitVersion inspect --format Values --variables SemVer

  -p, --path         The path to inspect, defaults to current directory.

  -f, --format       (Default: Json) The output format (Json, KeyValue or 
                     Values).

  -v, --variables    

  --verbose          Prints all system messages to standard output.

  --log              Specify log file for system messages.

  --help             Display this help screen.

  --version          Display version information.

verb: inject-buildserver

Example of one of the utility inject- verbs. Similar verbs: inject-msbuild, inject-process, inject-assembly-info.

USAGE:
Normal scenario, will detect build server automatically:
  GitVersion inject-buildserver

  --verbose            Prints all system messages to standard output.

  --log                Specify log file for system messages.

  --help               Display this help screen.

  --version            Display version information.

[serra]

@JakeGinnivan please review this first suggestion for a new CLI.

I investigated your verb-style proposal and see several advantages over the current option-only cli.

From what I can tell from this short spike so far, it would result in much cleaner and smaller codebase wrt argument parsing.

[serra]

@JakeGinnivan please review. I think this command style would be a good one and will lead to a much cleaner cli and codebase than my previous options-only spike.

I could implement this quite quickly for all verbs, but I'd like some feedback on the verbs and would appreciate some advice on how to write end-to-end tests for this.

Thanks!

[JakeGinnivan]

I just want to set expectations on this, I don't think it will land very quickly. I want to spend a few hours playing around with different options and comes up with pros/cons. And not really fussed on the actual code, I still am not sure if I think we should break compatibility or try and modify the old one.

Just giving the heaps up that I will probably change my mind and we need quite a few people to input into this before we commit to the decision. I will likely take this spike and the other then open a new issue to discuss the options. Appreciate the work you are doing on this though!

[serra]

That's OK @JakeGinnivan. Don't mind the code; I used it to try one of the other suggestions and create a verb-style cli.

It is hard to discuss the CLI without knowing the direction of GitVersion.exe. I reconstructed the verbs from what I perceived to be the current functionality exposed by GitVersion.exe.

My two cents:

• If you don't mind breaking changes, think this verb thing over some more, create a minimal subset and take it away
• If you do mind breaking changes, take the options-only approach (with whatever library you deem fit).

[asbjornu]

@serra I quite like the look and feel of your proposal. It feels more coherent and extensible than the current, at least. In terms of backwards compatibility, my vote is to break it on v4.0 so we can get a more robust and flexible foundation to build on for the future.

The CommandLine library and the code to implement it looks nice, but have you had a look at the .NET Command Line Interface? Seems to be a pretty safe choice going forward, but I haven't poked around nor had a look at how the code or output actually is.

[serra]

@asbjornu I did not check out the .NET Command Line Interface. I'm open to use to any library. IMO most important is to decided if/when to break the cli API, then draft up a proposal (similar to this one) and from there pick a library and move forward.

[dazinator]

Just an idea, we could always implement the new cli in a seperate project alongside the existing one, distribute both cli exe's (the old and the new) in the nuget package, and mark the old one as obsolete? This will mean people who call the old exe explicitly could still use it for a time, and this wouldn't need to be a major version increment / breaking change necessarily..

[asbjornu]

@JakeGinnivan, @serra: Seeing all the problems we have related to the CLI, / causing problems on POSIX systems, etc., it would be awesome to get a move on this new CLI. I think a command-based approach is awesome and absolutely believe that the new CLI needs to be a breaking change worth doing a major version bump for. If we can provide some backward compatibility, I feel that should be an added bonus, but not a requirement.

Thoughts?

[serra]

I have not got much time to spare atm; I'll try to catch up a bit tomorrow. It's been a long time since I've been working on this and I probably need an hour or so just to get started again.

[asbjornu]

@serra Awesome! That would be highly appreciated! ❤️

[serra]

@asbjornu I'm sorry but I'm not able to put as much time into this as needed. The checklist in the description of this PR still holds IMO. I think we could build this in a couple of days; especially if some of the core committers join in.

My experience from this spike was that it is doable and could be a significant improvement of the code base. At the time of the spike, the CLI code seemed hacky, but the tests were OK and helpful. It was pretty clear what I broke. Makes for a rewarding refactoring session.

I like @dazinator 's suggestion of creating two CLI wrappers in a separate project, but would suggest a time window for supporting both as I don't think it's a maintainable path for the long run.

When going for a breaking change, I'd strongly suggest reconsidering our console output too; IMO we should consider all default console output to be processable by other commandline tools. So no log/debug/warning messages UNLESS there is an exception, the --verbose switch is set or the selected verb+options specify it.

I'll keep an eye on the project, but I'm really uncertain about my availability to contribute.

[JakeGinnivan]

I just wanted to raise this again. I want to either close down these PR's or at least one of them but also take any learnings we have got and turn this into something actionable.

Can we create a legacy shim to translate old args into new ones or something like that?

[asbjornu]

@JakeGinnivan: As we've agreed on having a command-style CLI going forward, I think this can be kept open, as @serra has done a great job of documenting how it might look like. If we had something like what's suggested here implemented, I think the CLI would have a solid foundation that would stand the test of time and xplat much better than the current one.

Translating from the old scheme to the new one shouldn't be too hard, I think, since the old one is mostly not command-based. Those that are command-based, such as init, will probably stay mostly the same and thus be backwards compatible themselves.

The only way to guarantee backwards compatibility is to have 100% test coverage for the current CLI implementation, though. How far off the mark are we there?

[serra]

(This is from the top of my head)

There were some issues with getting a fully backwards compatible version of the cli. E.g. usage of '/' instead of '--' and casing. I was not able to workaround all of these. My work on #572 showed that these issues were caught by the test suite though.

100% backwards compatibility on the CLI is not easy to achieve and I'm not sure if it's worth pursuing. I'd opt for breaking the CLI and clearly document where backwards compatibility is broken.

[asbjornu]

@serra: Good points. How do you feel about the test coverage? Is it currently sufficient, or should we increase it to get a more complete view of what breaks with a new CLI, so we can document (all of) it?