`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.