Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Refactor and port CLI from Docopt to Clap (#2066) #6356

Merged
merged 5 commits into from
Sep 5, 2017

Conversation

axelchalon
Copy link
Contributor

@axelchalon axelchalon commented Aug 22, 2017

  • Previously we had the flags, arguments and subcommands in a text file for the help message, and once more in mod.rs to parse the command line. Now, mod.rs is the single source of truth and it also contains the usage and help messages of the flags/arguments/subcommands included. We generate the help message automatically based on this, with the default value included like before.
  • Subcommands and sub-subcommands now also have their own help messages : parity account --help, parity account list --help. Their help messages are very scanty for now, so some (more) help would be welcome. The help messages of the subcommands are generated by clap.
  • As per Clap specs, global ("top-level") arguments (for example --chain) must now come before the subcommand, i.e. parity snapshot --chain=dev mysnapshot.dump won't work; it needs to be parity --chain=dev snapshot mysnapshot.dump. The --help message was modified to convey this: parity [options] snapshot <FILE> for example. This is a breaking change, I believe. Likewise, subcommand-specific arguments aren't allowed before the subcommand anymore.
  • Because we want to still have the global arguments available when using subcommands, we cannot use the argument value syntax when value is the name of a subcommand; e.g. parity --ui-path signer won't work because signer is the name of a subcommand, and Clap parses this as running the signer subcommand with the global argument ui-path having an empty value. For those special cases we need to use the argument=value syntax: parity --ui-path=signer. I edited the corresponding test. (also filed an issue on clap, Argument value should take precedence over subcommand with the same name clap-rs/clap#1031) [EDIT: Clap fixed the issue]
  • For the record, the macro allows only two levels of subcommands, for example parity account import.
  • Made the naming more consistent: flag_s are CLI switches (boolean) such as (--no-config), arg_s take values (--chain dev, <FILE>). I renamed some arguments because of this.
  • I had to discard the internal options --can-restart and --force-direct from the help message because these flags are not parsed by clap but parsed beforehand, and the help message is generated based on clap arguments. If it is preferable to leave their help messages in the --help output, let me know and we could add them to the clap flags (even though their values from the clap Args struct wouldn't be used anywhere).
  • I split the --password argument depending on what command/subcommand it's for: arg_signer_sign_password, arg_wallet_import_password, arg_account_new_password, arg_password (the latter bing the only one of those arguments to be a vec), and removed the legacy option flag_warp, as its value was systematically the opposite of flag_no_warp (both values are fetched from the config file entry "warp"); I switched to using flag_no_warp instead of flag_warp on the one instance in the codebase where flag_warp as used. Also, the legacy flag flag_dapps_apis_all is now like all the other flags a bool, not an Option<bool>.
  • It might not be clear for the users that the config.toml files in parity/cli are only used for test purposes, so I moved them to a test subfolder.
  • The default values printed in the --help message for the options do not take into account the config file parameters; they are the built-in defaults. But I think it was also this way with docopt.
  • The syntax for multiple arguments is --argument value1 value2, although it would be possible to use commas as delimiters with clap instead.

Here's the --help output: https://gist.github.com/axelchalon/71e1f34019771f444d8380b21a88e2c8

Subcommand and subsubcommand --help output: https://gist.github.com/axelchalon/7a6762be783094a455a0e1b4a5181e88

...and --version output: https://gist.github.com/axelchalon/2639d0a3b1628c57afd242ee3163c430

@axelchalon axelchalon added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 22, 2017
@parity-cla-bot
Copy link

It looks like @axelchalon signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@@ -21,383 +21,941 @@ use dir;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unused import: dir
--> parity/cli/mod.rs:20:5
|
20 | use dir;
| ^^^
|
= note: #[warn(unused_imports)] on by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@debris
Copy link
Collaborator

debris commented Aug 22, 2017

Here's the --help output: https://gist.github.com/axelchalon/71e1f34019771f444d8380b21a88e2c8

Can you wrap it to 100 or 120 characters per line? this function might be helpful

Subcommand and subsubcommand --help output: https://gist.github.com/axelchalon/7a6762be783094a455a0e1b4a5181e88

That's a huuuge improvement!

As per Clap specs, global ("top-level") arguments (for example --chain) must now come before the subcommand

I can't talk for others, but I'm totally fine with this change

I will review it further tomorrow

@keorn
Copy link

keorn commented Aug 23, 2017

Cool.
--can-restart and --force-direct should remain in help (all options should be there).

@rphmeier
Copy link
Contributor

Closes #2066

@rphmeier
Copy link
Contributor

I would suggest that the help message for parity --help exclude all flags which exist only for subcommands if not already done.

@axelchalon
Copy link
Contributor Author

axelchalon commented Aug 25, 2017

Yes, the parity --help output only displays the help of "top-level" flags/arguments.

This being said, I just noticed that some of those top-level options are actually only used for subcommands (namely all the "Import/export" options except for, afaik, "flag_no_seal_check"), so I moved them to the subcommand's options. Let me know if there are other top-level options that are subcommand-specific!

I updated the gist of the parity --help output.

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

}
}

CMD cmd_snapshot
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should those two be grouped together?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like:
parity snapshot take and parity snapshot restore

Right now while parity snapshot is self-describing, I had to check the docs what parity restore is supposed to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, makes sense! I too think it'd be better this way, but then the usage would change from the current version (currently it's parity snapshot and parity restore; I didn't change the name of the subcommands or options, simply refactored). It wouldn't be hard to find the new subcommand, but it'd be a breaking change in case people have scripts running with the current syntax for example. maybe we can keep this PR a refactoring one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A separate PR is fine for me, although we already break backward compatibility by disallowing options after the command name.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 5, 2017
@gavofyork
Copy link
Contributor

@5chdn please work with @axelchalon for the 1.8.0 release's changelog - this will break quite a lot of installations.

@gavofyork gavofyork merged commit be745f7 into master Sep 5, 2017
@gavofyork gavofyork deleted the refactor-cli-clap-2066 branch September 5, 2017 11:30
@5chdn 5chdn added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Sep 5, 2017
@5chdn 5chdn mentioned this pull request Sep 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants