Code mess

[Meziu]

The code is a mess, between reading the arguments and weird panics (especially without having an -help option), it is a mess to use for anybody but me. It should be rewritten with the use of some fancy crate for CLI applications, but for now i just need it to work.

[ian-h-chamberlain]

To help with this issue I spent a bit of time investigating some different CLI parsing libraries. This tool is a bit of a special case since we want to pass through most unknown args directly to cargo, and most CLI libraries exit on failure for unknown options...

• clap 3.x seems promising, but requires -- to pass args that start with -- for now: https://github.com/clap-rs/clap/issues/1538
• https://github.com/blyxxyz/lexopt uses more of a "stream parsing" model where each arg is handled individually. This seems like it could work with a bit of tweaking and use of the Parser::raw_args() API, but again docs are a bit sparse and some APIs haven't been "released" yet. Also, no auto-generated help string :(
• https://github.com/google/argh seems pretty good, but doesn't support using "free" args alongside subcommands, which makes it tricky for passthrough cargo commands
• https://github.com/ksk001100/seahorse looks promising. Not much customization for help strings available but seems to support passthrough args better than other libraries

Other possibly useful crates:

• https://github.com/crate-ci/escargot seems decent for actually executing Cargo, but may not be as customizable as we want. Its docs are a bit sparse as well
• https://github.com/crate-ci/clap-cargo provides a few helper args for use with clap. Not sure if this one is really helpful in our case.

This isn't really a call for any specific direction, but I thought I'd dump what I found here for future reference. If no one else is looking at it, I could start working on this issue or at least draft a PR with what the CLI might look like if there's interest. I know there's lots of other work to be done too so if you'd rather I hold off working on it that's fine too :)

[AzureMarker]

Clap 2 plus structopt is my go-to

[ian-h-chamberlain]

Clap 2 plus structopt is my go-to

Yeah, I kind of dismissed it because it's in maintenance mode, but it's possible the clap 3 bug I linked isn't present there. I can probably check pretty easily if it would work as a proof of concept, and then hopefully the migration to clap 3 would be pretty easy once fixed upstream.

[AzureMarker]

Maintenance mode is still good, especially since (afaik) most of the ecosystem uses it.

Plus https://www.rust-lang.org/what/cli directly references it in the getting started example.

[ian-h-chamberlain]

Nice! This sort of works, with some caveats:

use clap2::AppSettings;

use structopt::StructOpt;

#[derive(Debug, StructOpt)]
pub struct Cli {
    #[structopt(subcommand)]
    command: Command,
}

#[derive(Debug, StructOpt)]
#[structopt(global_settings = &[
    AppSettings::ArgsNegateSubcommands,
    AppSettings::TrailingVarArg,
    AppSettings::AllowLeadingHyphen,
])]
pub enum Command {
    Run {
        #[structopt(flatten)]
        link_args: LinkArgs,

        #[structopt(multiple = true)]
        #[structopt(allow_hyphen_values = true)]
        cargo_args: Vec<String>,
    },

    // NOTE: need to check that subcommand is actually a subcommand,
    /// Run any other cargo command.
    #[structopt(external_subcommand)]
    Unknown(Vec<String>),
}

#[derive(Debug, StructOpt)]
#[structopt(unset_setting(AppSettings::AllowLeadingHyphen))]
pub struct LinkArgs {
    #[structopt(long)]
    // NOTE: seems like `allow_hyphen_values` has no effect here so we would
    // need to do manual validation of the value, and error messages might
    // not be quite as pretty...
    #[structopt(allow_hyphen_values = false)]
    link_address: Option<String>,
}

Maybe once that issue gets fixed we can upgrade to clap3 and get a slightly nicer behavior, but it's so recent that it will probably take a bit. I think this would do in the meantime and get a pretty decent result that's a bit more robust over the current implementation.