Discussion: CLI UI refactoring

[mroth]

WIP SAMPLE CODE -- BROKEN DO NOT MERGE.

For discussion with @matthutchinson (at your convenience! a slow burn on this makes sense, no rush), beginning of a patch that refactors the CLI UI of lolcommits to be significantly more modular and easy to understand, byusing subcommands. Previously, there was only one lolcommits "command", and actions were emulated by a bunch of logic around now different option flags were treated. All flags were global so it was unclear what options were compatible with which actions, etc. Now, with subcommands:

$ lolcommits --help
Usage:
    lolcommits [OPTIONS] SUBCOMMAND [ARG] ...

Parameters:
    SUBCOMMAND                    subcommand
    [ARG] ...                     subcommand arguments

Subcommands:
    enable                        Install lolcommits for current repository
    disable                       Uninstall lolcommits for current repository
    capture                       Capture image for most recent git commit
    history                       Historic archives of captured moments
    devices                       Detect and list attached camera devices
    plugins                       List or configure lolcommits plugins

Options:
    --version                     display version and exit
    --debug                       debug output (default: $LOLCOMMITS_DEBUG)
    -h, --help                    print help

$ lolcommits capture --help
Usage:
    lolcommits capture [OPTIONS]

Options:
    --device NAME                 Optional device name, see `lolcommits devices`.
                                   (default: $LOLCOMMITS_DEVICE)
    -a, --animate SECONDS         Enable animated .GIF capture for duration.
                                   (default: $LOLCOMMITS_ANIMATE, or 0)
    -w, --delay SECONDS           Delay capture to enable camera warmup.
                                   (default: $LOLCOMMITS_DELAY, or 0)
    --fork                        Fork capture process to background.
                                   (default: $LOLCOMMITS_FORK, or false)
    --stealth                     Capture image in stealth mode, e.g. no output.
                                   (default: $LOLCOMMITS_STEALTH, or false)
    -h, --help                    print help

$ lolcommits plugins --help
Usage:
    lolcommits plugins [OPTIONS] SUBCOMMAND [ARG] ...

Parameters:
    SUBCOMMAND                    subcommand
    [ARG] ...                     subcommand arguments

Subcommands:
    list                          List all available plugins
    config                        Configure a plugin
    show-config                   show config file

Options:
    -h, --help                    print help

$ lolcommits plugins list
# list of plugins outputted...

$ lolcommits plugins config slack
# etc...

Since we haven't hit v1.0 yet, we have a chance to make breaking changes to the UI. So on the "road to 1.0" along with the plugin extraction, I'd like to bring this up for consideration.

The bin/lolcommits was one of the messiest areas of the code, so this also starts on a refactor and cleanup (but trying to confine to the UI changes for now).

We get a lot for free from using Clamp here, such as automatic handling of environment variables linked to option flags (with validation of content type). This removes quite a bit of boilerplate from the code and I believe makes things more reliable.

Main purpose of this PR at present is to have a discussion about the UI tradeoffs here.

---

If we decide we do want to go down this path, some other things we'll need to discuss:

• This sets the stage for each of the commands to be broken into its own file and isolated, that can happen pretty quickly but I didn't want to do it just yet so we could more easily see where the duplication and redundancy was after porting over the CLI. Obviously the BDD tests would need to be updated as well.
• This breaks the ability to pass a set of default options to the Installer stub on enable. Previously this took advantage of the side effect that all flags were global. While we could certainly duplicate the flags for enable, I think it's worth considering a different approach -- I'm not convinced putting UI flags in the stub is a good idea. (At a minimum, we'd probably want to change to installing these options as environment variable sets in the stub instead.)
• Rationalize what is going on with change_dir_to_root_or_repo!. I think there might be a better way to accomplish this, but need to look into what the original impetus was. (Either way, it should probably be moved into a library and out of the CLI logic).
• Right now, this introduces one small dependency, but does not allow us to eliminate the Methadone dependency, since we rely on Methadone::CLILogging across the application. We can preserve this dependency for logging but perhaps worth thinking about whether something else fits the needs better. In any event, we also would need to refactor the CLILogging imports better, since previously it was Included into global scope in the main bin and all the places we called it sprinkled across the code seem to just rely on it being present.

P.S. Out of scope for this initial PR, but it makes me realize that it may be worthwhile longer term to refactor and rationalize the API of lolcommits outside of the command line binary. It should probably be possible to move all the logic into the library such that the gem could be used from within Ruby with signature such as Lolcommits.action_name(path, options) taking a string and a defined Options struct, and the CLI code would simply then handle constructing the correct Options and displaying errors that are bubbled back up. This is obviously a fairly large project however.

[matthutchinson]

Wow, looks great, I've not seen or used Clamp before.. I used sub in the past to organise subcommands, but this seems more up to date and easier to integrate.

I like the how Clamp::Command can associate ENV vars with flags and the fact the gem has no other dependencies.

I'm all for these changes - I agree that bin/lolcommits has always been a bit of a dumping ground. I added a couple of inline comments, but here are a few more questions;

• Do we still need methadone after switching to Clamp? Clamp deals with opts/flags/sub commands and ENVs' - are we keeping Methadone around for testing/logging convenience?
• Would it make sense to move all the Clamp::Command classes to their own files within lib/lolcommits/cli ?

I'm still working away on plugin extraction - got a bit sidetracked basically rewriting the Twitter plugin from scratch.. and the arrival of a baby - but should be back on track soon.

[matthutchinson]

👍

[matthutchinson]

The options to Installation.do_enable(options) end up just being a string, they are only used to populate the hook script with some default capture_args here ... so maybe you can get them from ARGV directly?

[matthutchinson]

Tests seems to be failing due to some Rubocop issues and some syntax errors; https://travis-ci.org/mroth/lolcommits/jobs/232434535 (in lib/lolcommits/cli/ui.rb)

[matthutchinson]

Also should mention - I don't think its too important to maintain exact parity or arg format and names on every command - EXCEPT the capture command - which lives in many git hooks around the world..... When I refactored to fully use Methadone (ditching Choice) I wasn't afraid to change a few option names where it made sense.

-- tho obviously the wiki/README would need updating 📖

[mroth]

Do we still need methadone after switching to Clamp? Clamp deals with opts/flags/sub commands and ENVs' - are we keeping Methadone around for testing/logging convenience?

After this change Methadone is only adding the CLILogging. We could factor this out and use our own logger or a different gem, but I'd like to keep that separate. Another thing to consider that I alluded to in my longer notes is that right now I believe the logging is sprinkled across the files, so we need to figure out the import situation a little bit.

Would it make sense to move all the Clamp::Command classes to their own files within lib/lolcommits/cli ?

Absolutely. The feature tests could be broken up this way too. For the initial spike I wanted to leave it in a single file as it made it easier to see where the duplication was, would want to take a few structural passes on it to try to DRY things up.

Also should mention - I don't think its too important to maintain exact parity or arg format and names on every command - EXCEPT the capture command - which lives in many git hooks around the world.....

The core change is that lolcommits --capture would unavoidably become lolcommits capture, the rest of the flags would be the same. I think this is something that could be handled with a "hidden" global --capture flag in Clamp, which manually redirects to the appropriate subcommand and perhaps emits a deprecation warning to stderr. Although I'd have to look into how to get the other flags passed along.

arrival of a baby

I know! commented on the Instagram photo -- so awesome. 👶 ...Is he writing Ruby yet? He can have commit rights as soon as he can type.

[matthutchinson]

After this change Methadone is only adding the CLILogging. We could factor this out and use our own logger or a different gem, but I'd like to keep that separate.

Given the pains of maintenance usually result from external gems, if we can build our own simple logger, I'd prefer that (and ditch Methadone)

The core change is that lolcommits --capture would unavoidably become lolcommits capture, the rest of the flags would be the same. I think this is something that could be handled with a "hidden" global --capture flag in Clamp

Sounds like a plan 👍 for backwards compatibility - I think we definitely need to continue supporting the --capture and -c options (with args)

Is he writing Ruby yet? He can have commit rights as soon as he can type.

No Ruby just yet, but we've watched some RailsConf 2017 talks together already. He's had me up since 4AM this morning 😪 !

[mroth]

Given the pains of maintenance usually result from external gems, if we can build our own simple logger, I'd prefer that (and ditch Methadone)

Makes sense to me. Could probably get most of what we need just by wrapping the standard Logger in Ruby stdlib. Do you think this would need to be part of this change, or a separate refactoring?

Sounds like a plan 👍 for backwards compatibility - I think we definitely need to continue supporting the --capture and -c options (with args)

Didn't seem possible to do this easily via Clamp directly, but I put in a hack in 8a8c7c1 that I think handles it with a bit of brute force simplicity. :-)

[mroth]

At this point, I've gotten it to the point where there are only two things remaining to all tests passing:

1. Lack of ability to pass additional options to enable that get stored in the hook. The more I think about this, the more I'm skeptical about supporting it in the future. A better long-term solution would be for us to allow for flag defaults to be stored in an actual config file -- people could still edit the hook file manually if they wanted, but I'm not sure what precedent we set by having this workaround "official" in a v1.0 release. (In any event, I can hack in some sort of compatible mode for it in the current PR if desired, but wanted to start more discussion on this in advance.)

2. A weird bug with the interactive config process crashing, having some difficulty figuring out what is going on there.

(Things are certainly workable enough now that this branch can be used for messing around and seeing how the UI of the commands "feels" and refining it and the name choices.)