clap derive support missing/broken

[virtualritz]

I have this:

use clap::Parser;
use twelf::config;

#[config]
#[derive(Parser)]
#[clap]
struct Config {
    #[clap(
        long,
        short = 's',
        help = "The server to connect to",
        display_order = 0
    )]
    host: Option<String>,

    #[clap(
        long,
        short,
        help = "The port to use",
        display_order = 1
    )]
    port: Option<u16>,
}

I expect a help like this:

USAGE:
    foo [OPTIONS]
    
OPTIONS:
    -s, --host <HOST>                        The server to connect to
    -p, --port <PORT>                        The port to use
    -h, --help                               Print help information

But I get this:

USAGE:
    foo [OPTIONS]

OPTIONS:
    -h, --help                               Print help information
        --host <host>
        --port <port>

I.e. it seems all clap-derive macro-related info is somehow ignored/lost.

I presume one key issue is that #[config] also creates a parse() method that somehow clashes with the one the #[derive(Parser)] wants to generate.

[bnjjj]

hmm yes I think the clap arguments are dropped, do you use the function provided by twelf to declare your clap commands and arguments ? Or do you use directly clap ?

I need to fix it ASAP, if you have a smal reproductible main to copy paste me here it would be helpful for me :)

[virtualritz]

I need to fix it ASAP, if you have a smal reproductible main to copy paste me here it would be helpful for me :)

The struct I posted can just be copypasta'd into a main.rs. Add the call to Config::parse() which usually calls clap's parse() and run this to see the help with broken/missing bits & bobs.

[bnjjj]

A new version of twelf has been released on 0.4.0 to support clap derive. If you want an example of how it works just check here https://github.com/bnjjj/twelf/blob/master/twelf/examples/clap_derive.rs

If you need something else feel free :)

[virtualritz]

Please reopen this.

I think you really need to test with all (or a lot more) clap_derive attributes before this is fixed. :)

It seems the config macro wraps members of a marked struct into Option. I think this is not a good idea. The user should do this explicitly. If they don't, use Default::default(), or expect a default attribute. Like clap.

Specifically clap allows one to omit Option for members which are still optional by specifying a default. Example:

#[derive(Parser, Debug)]
#[clap(author, version, about, long_about = None)]
struct Config {
    #[clap(
	long,
	default_value_t = String::from("localhost"),
        help = "Documentation inside clap, to specifiy db_host",
    )]
    db_host: String,
    ...
}

Here db_string is guaranteed to have a value but is still an option (--db-host can be omitted).
Because of this the user can use Config.db_host directly in their code w/o unwrap() and w/o any error handling ambiguity that would come with the latter.

Adding one line to twelf/examples/clap_derive.rs like above already breaks compilation (this is clap_derive complaining about the Option that db_host got wrapped into by the twelf::config macro):

error: default_value is meaningless for Option
  --> twelf/examples/clap_derive.rs:14:9
   |
14 |         default_value_t = String::from("localhost"),
   |         ^^^^^^^^^^^^^^^

[bnjjj]

Fixed and published as v0.5.0 sorry for the delay, I have a newborn at home and it takes me a lot of time these days :p

[bnjjj]

Tell me if it fixes your project.

[virtualritz]

I'm sorry I'm just getting to testing this now.
Thanks heaps for the fix! I will provide feedback asap.

[virtualritz]

Here is another example that doesn't work as expected:

use clap::{CommandFactory, Parser, Subcommand};
use serde::{Deserialize, Serialize};
use std::{
    fs::File,
    path::PathBuf,
    ffi::OsStr,
};
use twelf::{config, Layer};

pub const HELP_TEMPLATE: &str = "{bin} {version}

{about}

USAGE:
    {usage}

{all-args}
";

#[config]
#[derive(Parser, Debug)]
#[clap(
    help_template = HELP_TEMPLATE,
    version,
    about = "Twelf Test",
)]
pub struct CliOptions {
    #[clap(flatten)]
    pub global: GlobalOptions,

    #[clap(subcommand)]
    pub command: Command,
}

#[derive(Parser, Debug, Deserialize, Serialize)]
#[clap(rename_all = "kebab-case")]
pub struct GlobalOptions {
    /// Host name or IP address
    #[clap(display_order = 0, short = 'o', long, default_value_t = String::from("localhost"), env = "TB_HOST")]
    pub host: String,

    ///  Port number
    #[clap(
        display_order = 1,
        short,
        long,
        default_value_t = 4242u16,
        env = "TB_PORT"
    )]
    pub port: u16,
}

#[derive(Subcommand, Debug, Deserialize, Serialize)]
#[clap(rename_all = "kebab-case")]
pub enum Command {
    /// Place an order
    #[clap(display_order = 0, alias("p"))]
    Place(Place),

    /// Flatten all positions
    #[clap(display_order = 1, alias("f"))]
    Flatten(Flatten),
}

#[derive(Parser, Debug, Deserialize, Serialize)]
pub struct Flatten {
    #[clap(display_order = 0, long, short)]
    pub urgent: bool,

    #[clap(display_order = 1, long, short, conflicts_with = "urgent")]
    pub percentage: Option<f64>,

    #[clap(display_order = 2, long, short, conflicts_with = "urgent")]
    pub force_absolute_spread: Option<f64>,
}

#[derive(Parser, Debug, Deserialize, Serialize)]
pub struct Place {
    #[clap(short, long, parse(from_os_str), validator_os = path_readable_file, env = "TB_CONTRACT")]
    pub contract: Option<PathBuf>,

    #[clap(short, long, parse(from_os_str), validator_os = path_readable_file, env = "TB_ORDER")]
    pub order: Option<PathBuf>,

    #[clap(long)]
    pub execute: bool,
}

const TOML_CONFIG_FILE: &str = "config.toml";

fn main() {
    let matches = CliOptions::command().get_matches();

    let mut config_layers = Vec::with_capacity(2);
    if std::path::Path::new(TOML_CONFIG_FILE).exists() {
        config_layers.push(Layer::Toml(TOML_CONFIG_FILE.into()));
    }
    config_layers.push(Layer::Clap(matches));

    let _config = CliOptions::with_layers(&config_layers).unwrap();
}

fn path_readable_file(value: &OsStr) -> Result<(), String> {
    let path = PathBuf::from(value);//.as_ref();

    if path.is_dir() {
        return Err(format!("{}: Input path must be a file, not a directory",
                           path.display()));
    }

    File::open(&path).map(|_| ()).map_err(|e| format!("{}: {}", path.display(), e))
}

Running this with e.g.:

cargo run -- flatten --urgent

Gives me:

thread 'main' panicked at '`global` is not a name of an argument or a group.
Make sure you're using the name of the argument itself and not the name of short or long flags.', src/main.rs:20:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

When I remove the twelf stuff, everything works.

Also note that I need serde::{Deserialize, Serialize} on everything to make this compile.

I would very much like to only have some stuff in a config file. Not all. Specifically in the example code above only the GlobalOptions.

On that note: I do not understand why serde::Serialize is needed.

After all, I only want to read from a config file, not write to it.

[nyurik]

@virtualritz thx for a good example. I noticed you used TOML_CONFIG_FILE constant rather than taking config file name from the CLI. Would twelf be able to handle such case? Thx!

[virtualritz]

I'm not the author of ttwelf. I tried it and it wasn't working. So I opened the issue. I haven't revisited it since @bnjjj just reopened it after that Option issue I added. Once that is fixed I will try twelf again and may be able to answer your question.

[bnjjj]

@nyurik Yes it would be possible by directly using ArgMatches I did not already succeed to find a good solution for this issue though

[rteabeault]

Here is another example that doesn't work as expected:
I am also running into this issue.

[bglw]

👋

I don't have answers for the features that are unsupported, but I did come through this thread while I was working on setting up Twelf for a project so it might provide some value to show on the implementation I landed on, even if it isn't perfect.

First, I have my struct defined with clap derive (options.rs L6 -> L49), and have all fields marked as required = false, and defaults applied with the serde attribute rather than on clap — I don't want my CLI flags to have the default, I want my config as a whole to have the default. Adding defaults for clap meant that my CLI argument defaults would override config files.

Next I detect any config files and build up my layers with them and the environment (main.rs L24 -> L52). Below that I finalize using with_layers — since every key is not required and has a default the only errors thrown here are bad keys or invalid types.

The only non-ideal aspect is that I then have to validate an argument that is required manually — but I don't mind doing that in my use case, and I'm quite happy maintaining this setup going forward.

Apologies if this is slightly off topic for this issue, but it might help someone looking to get up and running with a clap derive implementation 🙂

[nyurik]

Update: clap-rs/clap#1206 has just been fixed, so you can iterate over args now.