Refactoring the cli #11702

wesrer · 2020-05-12T08:36:11Z

This PR refactors the cli macros with structopt and adds an inbuilt configuration generator that can save the current flags into a custom toml for future use.

dvdplm

When I run this I see a the argument list divided into FLAGS and OPTIONS where before this was not the case. I don't think that was intentional but I'm not sure what the plan is there: can you elaborate?
The list of flags/options does not contain the helpful "labels" we used to have, e.g.:

Snapshot Options:
    --no-periodic-snapshot
        Disable automated snapshots which usually occur once every 5000 blocks.

    --snapshot-threads=[NUM]
        Enables multiple threads for snapshots creation.

…are now printed as separate items far apart in the long list of options. Not sure how capable structopt is at grouping options together but I think it'd be important that we keep that.

dvdplm · 2020-05-25T09:01:04Z

parity/cli/subcommands.rs

+ )]
+ // FIXME: The original parser implementation had this as `Option<Vec<String>>` but this is not
+ // supported by structopt yet, referring to issue
+ // [#364](https://github.com/TeXitoi/structopt/issues/364)


Hmm, that's not quite how I read #364 – it seems to be quirky but not unsupported. Can you elaborate?

This is pretty awkward to distinguish in the CLI flag setting regardless so I'd treat empty list as None.

I believe the empty list was treated equivalently to None in the previous version, and it's the same in this version. All the previous tests are passing, so unless this was a case not tested, we are good

parity/cli/subcommands.rs

dvdplm · 2020-05-25T09:39:53Z

parity/cli/subcommands.rs

+ help = "Take a snapshot at the given block, which may be an index, hash, or latest. Note that taking snapshots at non-recent blocks will only work with --pruning archive"
+ )]
+ pub at: String,
+ // FIXME: check if the default is correct


How do you mean "default"? I don't think this should have a default (and it's required).

parity/cli/subcommands.rs

dvdplm · 2020-05-25T09:44:58Z

parity/lib.rs

@@ -241,10 +241,5 @@ pub fn start<Cr, Rr>(
 Cr: Fn(String) + 'static + Send,
 Rr: Fn() + 'static + Send
 {
- let deprecated = find_deprecated(&conf.args);


How is does deprecation work in this new version? It'd be good to have some internal documentation on how to best add/deprecate/remove cli flags/options/subcommands somewhere.

vorot93

Left a few comments. I'm currently digging the CLI story a bit deeper, and this PR with a few modifications may actually become mergeable short-term.

vorot93 · 2020-05-29T03:19:13Z

Cargo.toml

 term_size = "0.3"
 textwrap = "0.11.0"
 toml = "0.5.6"
+rust-embed = "5.5.1"


Any strong reason not to use standard include family macros?

I just found rust-embed to have a cleaner api. I believe internally, it expands to include macros

parity/cli/args.rs

vorot93 · 2020-05-29T03:38:14Z

parity/cli/subcommands.rs

+ )]
+ // FIXME: The original parser implementation had this as `Option<Vec<String>>` but this is not
+ // supported by structopt yet, referring to issue
+ // [#364](https://github.com/TeXitoi/structopt/issues/364)


This is pretty awkward to distinguish in the CLI flag setting regardless so I'd treat empty list as None.

parity/cli/globals.rs

parity/cli/args.rs

parity/cli/config.rs

parity/cli/globals.rs

wesrer · 2020-06-03T15:35:26Z

There are a few edge cases that are worth mentioning (will keep editing this comment with further findings):

If there is a default config that the user is using, and it has one of the boolean flags marked true, but for this instance they want it to be false, there is no way of expressing that since boolean flags are marked true if present and if absent uses the value of the default config.

dvdplm · 2020-06-04T06:34:13Z

* If there is a default config that the user is using, and it has one of the boolean flags marked true, but for this instance they want it to be false, there is no way of expressing that since boolean flags are marked true if present and if absent uses the value of the default config.

Not 100% sure I understand what you mean here. What is a "default config that the user is using"? and what is an "instance" in this context?

Do you mean that a user has prepared a config "template" of sorts that they use for several nodes and wish to override just a few option for one particular node instance? Can't they just --foo=false to override the foo: false in the config file?

wesrer · 2020-06-04T07:27:39Z

* If there is a default config that the user is using, and it has one of the boolean flags marked true, but for this instance they want it to be false, there is no way of expressing that since boolean flags are marked true if present and if absent uses the value of the default config.
Not 100% sure I understand what you mean here. What is a "default config that the user is using"? and what is an "instance" in this context?

Do you mean that a user has prepared a config "template" of sorts that they use for several nodes and wish to override just a few option for one particular node instance? Can't they just --foo=false to override the foo: false in the config file?

So essentially this version comes with a built in config generator flag that stores current flags into a file for future use - the assumption being people's configuration needs remain the same a majority of the time. However, once of twice, they may want to run a different config with a few altered flags - the usual way to do this is to just use their saved config and override flags, which is when this case arises. The user cannot --flag=false because boolean flags are interfaced as either present or absent. Thus --flag is true, and not having that option in the argument list is considered false. But in our case, we don't assume all absent flags are false, but rather they are to take the value of the provided config, which is where the problem arises.

dvdplm · 2020-06-04T08:00:26Z

The user cannot --flag=false because boolean flags are interfaced as either present or absent.

That is pretty surprising (not a good thing when it comes to UX). I think the user expectation is that the CLI should work (very) similarly to how it worked before and any deviation from that should be a clear improvement. New users will likely expect the openethereum CLI to work like most other command line tools, where --foo=false works as expected.

At this point I am not sure I understand what the idea is with this PR. Perhaps now is a good time to step back and write up a proper description on what the plan is, and how the CLI UI will work?

dvdplm reviewed May 25, 2020

View reviewed changes

wesrer marked this pull request as ready for review May 28, 2020 09:45

wesrer requested a review from vorot93 May 28, 2020 09:45

vorot93 suggested changes May 31, 2020

View reviewed changes

wesrer requested a review from vorot93 June 1, 2020 12:11

wesrer and others added 2 commits June 2, 2020 12:02

refactoring I have so far - cut out into separate file

5b99a53

Port to Clap 3.0

34658ec

vorot93 force-pushed the cli-refactor branch from 1315141 to 34658ec Compare June 2, 2020 09:04

sorpaas added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. M5-binaries 📦 External binaries (ethkey, ethstore, ethvm, etc.) labels Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring the cli #11702

Refactoring the cli #11702

wesrer commented May 12, 2020 •

edited

dvdplm left a comment

dvdplm May 25, 2020

vorot93 May 29, 2020

wesrer Jun 1, 2020

dvdplm May 25, 2020

dvdplm May 25, 2020

vorot93 left a comment

vorot93 May 29, 2020

wesrer Jun 1, 2020

vorot93 May 29, 2020

wesrer commented Jun 3, 2020 •

edited

dvdplm commented Jun 4, 2020

wesrer commented Jun 4, 2020

dvdplm commented Jun 4, 2020

Refactoring the cli #11702

Are you sure you want to change the base?

Refactoring the cli #11702

Conversation

wesrer commented May 12, 2020 • edited

dvdplm 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

Choose a reason for hiding this comment

vorot93 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

wesrer commented Jun 3, 2020 • edited

dvdplm commented Jun 4, 2020

wesrer commented Jun 4, 2020

dvdplm commented Jun 4, 2020

wesrer commented May 12, 2020 •

edited

wesrer commented Jun 3, 2020 •

edited