swellaby / rusty-hook

git hook manager, geared toward Rust projects
MIT License
206 stars 11 forks source link

CLI changes for v0.12.0 not compatible with all earlier scripts #129

Open calebcartwright opened 3 years ago

calebcartwright commented 3 years ago

As part of the recent batch of changes and bump to the unreleased v0.12.0, the CLI args were updated a bit as part of the switch to Clap.

Current versions of the hook scripts run rusty-hook run --hook "${hookName}" "${gitParams}" providing the git params as a positional argument that's empty in cases of hooks that have no args. This is problematic with clap which doesn't allow explicitly empty values by default.

The impact of this would be users currently using rusty-hook in multiple projects would experience rusty-hook failures after updating one project (and thus updating the rusty-hook cli to v0.12.0) in all other projects.

To address, we need to update the clap interface to accept that arg for legacy/BC purposes

Scandiravian commented 2 years ago

I tried modifying the RustyHookOpts to this

Run {
        #[clap(long)]
        hook: String,
        // Changed from #[clap(name = "git args", raw(true))]
        #[clap(name = "git args")]
        args: Option<String>,
    },

This allows the cli to be called as in v0.11 (rusty-hook run --hook "${hookName}" "${gitParams}")

I then made a small script to test out the solution

# test.sh
echo -e "1: $1\n2: $2\n3: $3\n"

And finally I configured the rusty hooks

[hooks]
commit-msg = "./test.sh %rh! 'hardcoded arg'"
# rusty-hook run --hook "commit-msg" "git_params"
# Output:
# 1: git
# 2: params
# 3: hardcoded arg

# rusty-hook run --hook "commit-msg" ""
# Output:
# 1: hardcoded arg
# 2: 
# 3: 

# rusty-hook run --hook "commit-msg"
# Output:
# 1: %rh!
# 2: hardcoded arg
# 3: 

From how I interpret the results, I think this would solve the issue of broken backwards compatibility. I'd like to get some feedback on whether this is a workable solution or there are any considerations, that I have forgotten to account for?

I can implement the changes and probably add some tests for the argument parsing as well during the weekend if this is acceptable :slightly_smiling_face:

calebcartwright commented 2 years ago

Yeah that's the direction I'd started heading in as well, but ended up stepping back from it both due to some other priorities and because I'm increasingly reconsidering whether it really makes sense to utilize clap in this context.

Clap is absolutely fabulous and greatly simplifies and streamlines the process of developing rich CLI tools, but in hindsight I just feel like it's too heavy for rusty-hook which is largely behind the scenes and rarely, if ever, used directly by a user. As such I really feel like we should go back to getopts and optimize for compile time over feature richness.

If you're up for looking into that it would be fantastic and most appreciated :pray: