CLI redesign (#58, #134, #140) #144

ilslv · 2021-10-20T07:40:27Z

Synopsis

In v0.10.0 CLI was preserved as is with error messages for unsupported options. But we want to rework it for more flexibility.

Solution

Redesign CLI and make it composable.

Checklist

# Conflicts: # src/parser/basic.rs # src/writer/basic.rs # src/writer/normalized.rs

ilslv · 2021-10-21T06:25:08Z

@tyranron I've finished implementing composable CLI for Parser, Runner and Writers, but I think we want an ability to additionally compose arbitrary user's CLI options. There are 3 possible options:

Versatile
Add Cucumber::cli<CustomCli>(self) -> (CustomCli, Self) which can be optionally call after Cucumber setup. This will allow for example to use CustomCli struct inside Cucumber::filter_run() method.
There is 1 downside: We will want to store optionally computed Option<cli::Opts<..>> to reuse it or compute for the first time in Cucumber::run_* methods. This will require to add 3 more generic parameters to Cucumber struct, which already has 5 (it sounds very bad, but in practice I've would say it's just a bit more unpleasant to read this code). Or Introduce some kind of builder pattern.

let (custom_cli, cucumber) = World::cucumber().cli::<CustomCli>();
cucumber.filter_run_and_exit(input, |_, _, _| {
        // reuse custom_cli
    })
    .await;

Less versatile
In addition to all Cucumber::(filter_)run(_and_exit) methods we can add _with_additional_cli prefix to all of them and return (CustomCli, impl Future<Output = ..>) from them. This api is less versatile, as we'll lose an ability to to reuse CustomCli in Cucumber::filter_run(_and_exit) methods, but code will be easier to navigate or refactor.

let (custom_cli, fut) = World::cucumber()
    .filter_run_and_exit_with_additional_cli::<CustomCli>(input, |_, _, _| {
        // no way to reuse custom_cli
    });
fut.await;

Most versatile, but maybe less intuitive to use
Instead of managing our CLI composition with user's one, we can shift this responsibility to user. So for Cucumber not to hijack the console, you'll need to provide needed CLI yourself.

let cli::Compose { left: custom_cli, right: cucumber_cli } = 
    cli::Compose<CustomCli, cli::Opts<_, _, _>>::from_args();

World::cucumber()
    // additional setup
    .filter_run_and_exit_with_cli(cucumber_cli, input, |_, _, _| {
        // reuse custom_cli
    })
    .await;

I would go with this third one, as the situation, where you would like to expand CLI with arbitrary options should be pretty rare.

tyranron · 2021-10-21T08:06:04Z

@ilslv why cannot we parametrize cli::Opts with CustomCli (defaulting to () or something like that), as we do for Parser/Runner/Writer CLIs? And make the whole parsed CLI accessible in filter_run or similar? To make judging the user may want to use standard options too, why not?

So, the Cucumber will be parametrized only with one Cli type always.

Regarding the interfaces, I imagine something like this:

World::cucumber()
    .extra_cli::<MyCustomCli>()
    .filter_run_and_exit(input, |_, _, _, cli| {
        // use cli
    })
    .await;

ilslv · 2021-10-21T08:08:39Z

@tyranron this can be problematic, when users want to use those options outside filter_run method. For example add --max-threads option and parameterise async runtime with it.

tyranron · 2021-10-21T08:13:18Z

@ilslv can you provide a code example?

ilslv · 2021-10-21T08:19:21Z

@tyranron

#[derive(StructOpt)]
struct MaxThreads {
    #[structopt(long)]
    max_threads: Option<usize>,
}

fn main() {
    let (custom, cli) = 
         cli::Compose::<MaxThreads, cli::Opts<_, _, _>>::from_args()
              .into_inner();
    tokio::runtime::Builder::new_multi_thread()
        .enable_all()
        .worker_threads(custom.max_thread.unwrap_or(10))
        .build()
        .unwrap()
        .block_on(async {
            World::cucumber()
                .run_and_exit_with_cli(cli, "features")
                .await;
        })
}

tyranron · 2021-10-21T08:47:27Z

@ilslv what about something like this?

#[derive(StructOpt)]
struct MaxThreads {
    #[structopt(default_value = "10")]
    max_threads: usize,
}

fn main() {
    let opts = cli::Opts<MaxThreads, _, _, _>>::from_args();

    tokio::runtime::Builder::new_multi_thread()
        .enable_all()
        .worker_threads(opts.extra.max_thread)
        .build()
        .unwrap()
        .block_on(async move {
            World::cucumber()
                .with_cli(opts)
                .run_and_exit("features/")
                .await;
        })
}

ilslv · 2021-10-21T08:53:25Z

@tyranron I don't quite understand why we would use that api, as it can introduce redundant .clone() and has no obvious benefits

#[derive(StructOpt)]
struct MaxThreads {
    #[structopt(default_value = "10")]
    max_threads: usize,

    #[structopt(default_value = "thread")]
    thread_name: String,
}

fn main() {
    let opts = cli::Opts<MaxThreads, _, _, _>>::from_args();

    tokio::runtime::Builder::new_multi_thread()
        .enable_all()
        .worker_threads(opts.extra.max_thread)
        .thread_name(opts.thread_name.clone())
//                                      ^ redundant
        .build()
        .unwrap()
        .block_on(async move {
            World::cucumber()
                .with_cli(opts)
//                          ^ no way to access `max_threads` or `thread_name`
                .run_and_exit("features/")
                .await;
        })
}

tyranron · 2021-10-21T09:37:17Z

@ilslv I don't take that as an argument, because despite your example, an user may want to use it both outside and inside .filter_and_run() closure too, so he still will need to do that cloning even having custom_cli decoupled.

And I don't see problems with cloning on setup. This is not a hot path. Trying to zerocopying here is kinda overcarring 😅

ilslv · 2021-10-21T11:45:36Z

FCM

Redesign and rework CLI (#144, #58, #134, #140)

- make CLI options  composable by adding `Cli` associated type to `Parser`, `Runner` and `Writer` traits
- switch to `structopt` crate as `clap` doesn't support generic flattening at the moment
- allow extend `cli::Opts` with a custom `StructOpt` deriver

Additionally:
- fix accidenatlly messed up imports style

tyranron

@ilslv design is neat and good 👍 Polishing and bikeshedding is still required, though.

tyranron · 2021-10-21T16:30:21Z

tests/wait.rs

@@ -18,7 +21,7 @@ async fn main() {
        .after(|_, _, _, _| {
            time::sleep(Duration::from_millis(10)).boxed_local()
        })
-        .run_and_exit("tests/features/wait");
+        .run_and_exit_with_cli(cli, "tests/features/wait");


Wait, why do we do it here? This should'nt be required. .run_and_exit("tests/features/wait"); shoud work just fine without a need to explicitly specify cli::Empty.

@tyranron wait test just became some kind of proving ground for me, where we can toss all available features, whether we need them or not.

@ilslv ok, but let's roll out something custom then, instead of cli::Empty.

tyranron · 2021-10-21T16:36:02Z

src/cucumber.rs

+    /// [`Step`]: gherkin::Step
+    pub async fn run_and_exit_with_cli<Cli>(
+        self,
+        cli: cli::Opts<Cli, P::CLI, R::CLI, Wr::CLI>,


We better place it inside Cucumber struct, so instead of introducing the whole new set of *run*_with_cli() methods, we will need the only .with_cli() when building and before running.

I've described problem with this approach in point 1 here. We can't just use cli::Opts<Cli, P::CLI, R::CLI, Wr::CLI>, because Cucumber generic parameters are bound inside impls, not directly on struct. This allows us to have impls like

impl<W> Cucumber<W, (), (), (), ()> { /// Creates an empty [`Cucumber`] executor. /// /// Use [`Cucumber::with_parser()`], [`Cucumber::with_runner()`] and /// [`Cucumber::with_writer()`] to be able to [`Cucumber::run()`] it. #[must_use] pub const fn custom() -> Self { Self { parser: (), runner: (), writer: (), _world: PhantomData, _parser_input: PhantomData, } } }

@ilslv I don't see any problems to bound them in a struct, though. In this case it would be quite handy.

tyranron · 2021-10-21T16:40:14Z

src/parser/basic.rs

+#[cfg_attr(doc, doc = "CLI options of [`Basic`].")]
+#[allow(missing_debug_implementations)]
+#[derive(StructOpt)]
+pub struct CLI {


Abbrevs still do better have PascalCase style, not UPPERCASE.

tyranron · 2021-10-21T17:05:11Z

src/parser/basic.rs

+pub struct CLI {
+    /// Feature-files glob pattern.
+    #[structopt(long, short, name = "glob")]
+    pub features: Option<Walker>,


We should mention here default value/behavior.

tyranron · 2021-10-21T17:09:59Z

src/cli.rs

-#[derive(clap::Parser, Debug)]
-pub struct Opts {
+#[derive(Debug, StructOpt)]
+pub struct Opts<Custom, Parser, Runner, Writer>


As we still need to specify all the parameters, I think it's better to move Custom to the last position and default it to cli::Empty.

tyranron · 2021-10-21T17:11:20Z

src/cli.rs

+        visible_alias = "scenario-tags",
+        conflicts_with = "regex"
+    )]
+    pub tags_filter: Option<TagOperation>,


This option and the one above have the same descriptions, which is quite confusing.

tyranron · 2021-10-21T17:14:22Z

src/cli.rs

+)]
+#[cfg_attr(doc, doc = "Composes two [`StructOpt`] derivers together.")]
+#[derive(Debug, StructOpt)]
+pub struct Compose<L, R>


I don't think we need it anymore.

If it's still may be useful in some way for library users, it should have an example in its documentation (or on a module level), describing the case.

I left it because it can be very useful for custom Writer wrappers. They can store their own cli options in addition to inner Writers.

tyranron · 2021-10-21T17:15:08Z

src/cli.rs

@@ -10,29 +10,106 @@

 //! CLI options.


It would be good to have a whole CLI idea description/overview on module level docs.

ilslv · 2021-10-22T05:15:05Z

@tyranron should we upgrade to 2021 edition and bump msrv right now?

tyranron · 2021-10-22T08:11:17Z

@ilslv

should we upgrade to 2021 edition and bump msrv right now?

Not in terms of this PR. I plan to do it separately after merging this. There will be quite a stuff to do.

# Conflicts: # codegen/src/lib.rs # src/lib.rs

tyranron · 2021-10-25T11:55:33Z

tests/wait.rs

            async move {
                w.0 = 0;
-                time::sleep(Duration::from_millis(10)).await;
+                time::sleep(time).await;


Disjoint capture really helped here 🎉

tyranron · 2021-10-25T12:05:44Z

src/parser/basic.rs

+pub struct Cli {
+    /// `.feature` files glob pattern.
+    #[structopt(long, short, name = "glob")]
+    pub features: Option<Walker>,


#144 (comment)

Add --verbose flag and remove legacy CLI options

f031fc2

ilslv added the enhancement Improvement of existing features or bugfix label Oct 20, 2021

ilslv added this to the 0.11 milestone Oct 20, 2021

ilslv self-assigned this Oct 20, 2021

ilslv added 7 commits October 20, 2021 12:27

Add --scenario-name and --scenario-tags CLI options

a4a8f62

Add --features option

f0220bb

Correction

696de67

Add Cucumber methods for additional user CLI

7b24407

Correction

d9c2589

Merge branch 'main' into 134-cli-redesign

ba2eccf

# Conflicts: # src/parser/basic.rs # src/writer/basic.rs # src/writer/normalized.rs

Corrections

0b58059

Corrections

d7bb724

ilslv added 4 commits October 21, 2021 12:11

Corrections and add todo_or_die crate

c48d942

Corrections

4119545

Lint

f0c65db

Try removing todo-or-die crate

7c47f3b

ilslv added 3 commits October 21, 2021 13:22

Add Custom generic parameter to cli::Opts

90cab88

Mention CLI in Features chapter of the book

2e83a94

Bump version to 0.11 and add CHANGELOG entry

d104147

ilslv mentioned this pull request Oct 21, 2021

Support pass-through for all arguments that cargo test supports #58

Closed

ilslv changed the title ~~Draft: CLI redesign (#134, #140)~~ Draft: CLI redesign (#58, #134, #140) Oct 21, 2021

ilslv marked this pull request as ready for review October 21, 2021 11:45

ilslv requested a review from tyranron October 21, 2021 11:46

Some corrections [skip ci]

75f542a

tyranron requested changes Oct 21, 2021

View reviewed changes

tyranron modified the milestones: 0.11, 0.10 Oct 21, 2021

Corrections

cf8e669

ilslv added 2 commits October 22, 2021 12:27

Move trait bound from impls to struct in Cucumber

d345c4c

Corrections

c2d254c

ilslv requested a review from tyranron October 22, 2021 10:04

tyranron force-pushed the main branch from db0f1a3 to 3541120 Compare October 25, 2021 11:30

tyranron added 3 commits October 25, 2021 14:43

Merge branch 'main' into 134-cli-redesign

04f2c6b

# Conflicts: # codegen/src/lib.rs # src/lib.rs

Restore broken imports

e0b85c0

Corrections

df487e1

tyranron approved these changes Oct 25, 2021

View reviewed changes

tyranron changed the title ~~Draft: CLI redesign (#58, #134, #140)~~ CLI redesign (#58, #134, #140) Oct 25, 2021

tyranron merged commit 4fa122a into main Oct 25, 2021

tyranron deleted the 134-cli-redesign branch October 25, 2021 17:11

tyranron mentioned this pull request Nov 5, 2021

Moving structopt to clap (clap_derive) #155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI redesign (#58, #134, #140) #144

CLI redesign (#58, #134, #140) #144

ilslv commented Oct 20, 2021 •

edited by tyranron

Loading

ilslv commented Oct 21, 2021

tyranron commented Oct 21, 2021 •

edited

Loading

ilslv commented Oct 21, 2021

tyranron commented Oct 21, 2021

ilslv commented Oct 21, 2021 •

edited

Loading

tyranron commented Oct 21, 2021 •

edited

Loading

ilslv commented Oct 21, 2021 •

edited

Loading

tyranron commented Oct 21, 2021 •

edited

Loading

ilslv commented Oct 21, 2021 •

edited by tyranron

Loading

tyranron left a comment

tyranron Oct 21, 2021

ilslv Oct 22, 2021

tyranron Oct 22, 2021

tyranron Oct 21, 2021

ilslv Oct 22, 2021 •

edited

Loading

tyranron Oct 22, 2021

tyranron Oct 21, 2021 •

edited

Loading

tyranron Oct 21, 2021

tyranron Oct 21, 2021 •

edited

Loading

tyranron Oct 21, 2021

tyranron Oct 21, 2021

ilslv Oct 22, 2021

tyranron Oct 21, 2021

ilslv commented Oct 22, 2021

tyranron commented Oct 22, 2021 •

edited

Loading

tyranron Oct 25, 2021

tyranron Oct 25, 2021

CLI redesign (#58, #134, #140) #144

CLI redesign (#58, #134, #140) #144

Conversation

ilslv commented Oct 20, 2021 • edited by tyranron Loading

Synopsis

Solution

Checklist

ilslv commented Oct 21, 2021

tyranron commented Oct 21, 2021 • edited Loading

ilslv commented Oct 21, 2021

tyranron commented Oct 21, 2021

ilslv commented Oct 21, 2021 • edited Loading

tyranron commented Oct 21, 2021 • edited Loading

ilslv commented Oct 21, 2021 • edited Loading

tyranron commented Oct 21, 2021 • edited Loading

ilslv commented Oct 21, 2021 • edited by tyranron Loading

FCM

tyranron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilslv Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyranron Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyranron Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilslv commented Oct 22, 2021

tyranron commented Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilslv commented Oct 20, 2021 •

edited by tyranron

Loading

tyranron commented Oct 21, 2021 •

edited

Loading

ilslv commented Oct 21, 2021 •

edited

Loading

tyranron commented Oct 21, 2021 •

edited

Loading

ilslv commented Oct 21, 2021 •

edited

Loading

tyranron commented Oct 21, 2021 •

edited

Loading

ilslv commented Oct 21, 2021 •

edited by tyranron

Loading

ilslv Oct 22, 2021 •

edited

Loading

tyranron Oct 21, 2021 •

edited

Loading

tyranron Oct 21, 2021 •

edited

Loading

tyranron commented Oct 22, 2021 •

edited

Loading