feat: first iteration of cli

[kristof-mattei]

In response to #35

Hoping to get some initial conversation started here about how we want to design the CLI.

How close do we want to mirror the original one? clap does not support multi-character short args (like -ab).

[xamgore]

Hello, thank you so much!

How close do we want to mirror the original one? clap does not support multi-character short args (like -ab).

I wouldn't bother then. If unknown arguments (like -df) raise an error, it's good feedback that something going wrong.

[Jeanbouvatt]

Shouldn't it be Option<String> ?

If you don't provide text_input you get an error, however if input_file is provided text_input could, and should remains unprovided.

Similarly should input_file be Option<PathBuf> ?

[Jeanbouvatt]

cargo run -- --help yield " -i, --input-file <INPUT_FILE> " instead of -ti

-ti feel weird though. One letter flag -i is a better alternative despite the retro-compatibility

[kristof-mattei]

I'm moving the inputs to a separate struct so we can use clap's required = true and multiple = false.

The new struct will have 2 Options

[Jeanbouvatt]

ngram_size, dedup_function, dedup_lim and window_size are all optional in python yake cli and in this rust implementation thanks to Config::default.
Maybe default values could provide a much easier to use cli.

[kristof-mattei]

This is a great comment. And a point that I have struggled with when designing CLIs with clap.

Where do you put the defaults? The CLI is a single struct that is all the CLI inputs, but then gets transformed into other structs with their own defaults.

[kristof-mattei]

In other words:

when the user looks at the help of ngram_size, they expect it to be optional.

BUT... it does have a default, i.e. 3 in this codebase.

So do we expect the user to see this 3? If so we should do

#[arg(short, long, default_value_t = Config::default().ngrams)]
ngram_size: usize

That way the default is shown as

  -n, --ngram-size <NGRAM_SIZE>    Max size of the ngram [default: 3]

However, if we make it an Option<usize>:

  -n, --ngram-size <NGRAM_SIZE>    Max size of the ngram

The default isn't shown.

[xamgore]

My cent: don't be scared to repeat the defaults, as in such case they will be visible to the user. Which is beautiful and allows tuning parameters. You may do something like this to ensure nobody will forget a field in the future:

impl From<ClapConfig> for Config {
  fn from(value: ClapConfig) -> Self {
    let ClapConfig { one, two, three } = value;  // if ClapConfig has a new field, it has to be added here

    // Config has a new field, it has to be added here
    Self { ... }
  }
}

[kristof-mattei]

@xamgore I've done something for which the CLI reads the defaults from yake's config, so any changes there propagate now.

[xamgore]

hi 😏, @Jeanbouvatt, could you put it under optional serde feature flag?

It's common for rust community to reduce the burden of dependencies. Like, if I don't need JSON at all, serde won't be compiled.

For that, a crate defines an optional feature, which is kinda synced with the dependency.

• If the feature is manually listed yake-rust = { version = "...", features = ["ineedjson"] }
• If the crate is listed as a dependency in user's Cargo.toml: [dependencies] serde = "*"

In any of those two cases, ineedjson will be turned on, and thus serde crate will be installed. Even more cfg!(feature = "ineedjson") will be true, and #[cfg(feature = "ineedjson")] macro will include the following code.

[Jeanbouvatt]

Sure! will do

[Jeanbouvatt]

Here it is

kristof-mattei#2

[xamgore]

Hey guys, if you feel like working with forked repository is a burden, @quesurifn can grant you permission for collaborating directly here.

[xamgore]

Ok, so we are basically waiting for kristof-mattei#3 being merged and ready to move on