Replace positional arguments with flags in CLI #2275

ljoss17 · 2022-06-07T10:05:22Z

Closes: #2239

Description

This PR adds flags instead of positional arguments to all CLI commands. Long flags have been used in order to avoid conflicts or confusing flags.

WIP

At commit 51852ae: Some short flags are still present for some commands, but a discussion in order to clarify if all the short flags will be removed or if some strategic short flags will be kept is ongoing.
At commit 88474db and a76599f: Removed all short flags.

PR author checklist:

Added changelog entry, using unclog.
~~Added tests: integration (for Hermes) or unit/mock tests (for modules).~~
Linked to GitHub issue.
Updated guide (e.g., guide/).

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

…ccordingly

plafer · 2022-06-08T18:11:23Z

Just a note not to forget to update the gm hermes cc command. This is the current output:

% gm hermes cc
"./hermes" create channel ibc0 ibc1 --port-a transfer --port-b transfer

ljoss17 · 2022-06-09T08:06:41Z

Just a note not to forget to update the gm hermes cc command. This is the current output:
% gm hermes cc
"./hermes" create channel ibc0 ibc1 --port-a transfer --port-b transfer

Yes, I will check all the commands which use hermes and update them with the new flags.

…nt upgrade guide page

hu55a1n1 · 2022-06-14T15:59:28Z

Just some pointers to help add unit-tests for clap CLI arg parsing -> clap-rs/clap#1355 (comment)

ljoss17 · 2022-06-15T15:03:06Z

relayer-cli/src/commands/keys/add.rs

+    fn test_keys_add_key_and_mnemonic() {
+        assert!(KeysAddCmd::try_parse_from(&["test", "--chain", "chain_id", "--key-file", "key_file", "--mnemonic-file", "mnemonic_file"]).is_err());
+    }
+}


Hey @hu55a1n1 @mzabaluev do these examples for unit-testing the clap arg parsing seem good to you ? Or do you have any feedback regarding unit-testing the clap arg parsing ?

This is great! Given how many changes are/ will be in this PR already, can we do the clap tests in a different PR?

This is exactly what I had in mind! 👌 Would be nice to get this in before relayer-cli v1. I agree it's better to do it in a different PR.

I was hoping there is some framework to provide some handy test macros, but I can't find any at the moment.
This will do, we can add more cases to exercise.

If you have time, consider creating some helper macros like:

assert_valid_cmdline!("--chain", "chain_id", "--key-file", "key_file", "--mnemonic-file", "mnemonic_file"); assert_invalid_cmdline!("--chain", "chain_id", "--key-file", "key_file", "--mnemonic-file", "mnemonic_file");

Thank you for all the feedback! I will check this as soon as this PR is merged and I work on the unit-tests.

mzabaluev · 2022-06-18T08:06:18Z

I have found trycmd, which looks like a useful tool to organize CLI tests in a self-documented way.

This could also augment our manually maintained command examples in the guide mdbook. Not in scope for this PR, though.

romac · 2022-06-24T09:19:41Z

Great stuff! Just one question: what's the rationale behind using --chan instead of --channel?

ljoss17 · 2022-06-24T09:30:43Z

Since we use only long flags, the idea was to reduce the flag length for cases which are still meaningful after being shortened. This was done for the flags --connection, --channel and --sequence, that became --conn, --chan and --seq.

romac · 2022-06-24T10:08:47Z

Is there an easy way to allow both --chan and --channel? (respectively conn/connection, seq/sequence)

ljoss17 · 2022-06-24T10:56:13Z

Is there an easy way to allow both --chan and --channel? (respectively conn/connection, seq/sequence)

Yes, with clap's alias. For example if we add the alias to the the channel_id in the QueryChannelEndCmd:

    #[clap(
        long = "chan",
        alias = "channel",
        required = true,
        value_name = "CHANNEL_ID",
        help = "identifier of the channel to query"
    )]
    channel_id: ChannelId,

Then both these tests will pass:

    #[test]
    fn test_query_channel_end_required_only() {
        assert_eq!(
            QueryChannelEndCmd{ chain_id: ChainId::from_string("chain_id"), port_id: PortId::from_str("port_id").unwrap(), channel_id: ChannelId::from_str("channel-07").unwrap(), height: None },
            QueryChannelEndCmd::parse_from(&["test", "--chain", "chain_id", "--port", "port_id", "--chan", "channel-07"])
        )
    }

    #[test]
    fn test_query_channel_end_required_only_alias() {
        assert_eq!(
            QueryChannelEndCmd{ chain_id: ChainId::from_string("chain_id"), port_id: PortId::from_str("port_id").unwrap(), channel_id: ChannelId::from_str("channel-07").unwrap(), height: None },
            QueryChannelEndCmd::parse_from(&["test", "--chain", "chain_id", "--port", "port_id", "--channel", "channel-07"])
        )
    }

romac · 2022-06-24T11:18:07Z

Sweet, then let's do this. Remains to decide whether we want the long (channel) or shortened (chan) version to be the default one? What do you think? I would vote for the long one being the default and the shorter one as alias, but would like to hear what you and others think?

ljoss17 · 2022-06-24T11:25:53Z

I have a preference for the shortened version, but I think my opinion is biased due to using them when replacing and testing the commands.
Should the aliases be added in the PR adding the parsing unit-tests, which I will work on as soon as this PR is merged ? Or should they be added in a separate PR which would only add the aliases ?

seanchen1991 · 2022-06-24T20:32:40Z

I would also agree with sticking with the long versions, if only to be more explicit. I think this applies to conn and connection as well.

romac · 2022-06-27T08:16:23Z

Should the aliases be added in the PR adding the parsing unit-tests, which I will work on as soon as this PR is merged ? Or should they be added in a separate PR which would only add the aliases ?

I would rather add the aliases to this PR to keep PR churn down, if that's okay.

mzabaluev

Good work!

I've reviewed "my turf"...

health-check, help

These were easy: no parameters, nothing to change!
So I took it upon myself to look at some more commands.

guide/src/commands/relaying/clear.md

guide/src/commands/path-setup/index.md

relayer-cli/src/commands/create/channel.rs

relayer-cli/src/commands/create/connection.rs

relayer-cli/src/commands/listen.rs

mzabaluev · 2022-06-27T14:36:24Z

Please also merge with recent master as there are some new conflicts.

romac · 2022-06-28T07:58:25Z

What's the reasoning behind having --a-chain instead of --chain-a? I am asking because the latter reads better to me.

ljoss17 · 2022-06-28T08:10:10Z

What's the reasoning behind having --a-chain instead of --chain-a? I am asking because the latter reads better to me.

The choice of prefixing --a- and --b- is due to the following discussion in the ADR PR : #2306 (comment)

…mmand

…'s help for autogenerated arguments (eg. --help)

…case letter

romac

Amazing work @ljoss17, thank you so much! Many thanks to all reviewers as well :)

informalsystems/hermes#2275

* Updated CLI commands to take flags everywhere and updated e2e tests accordingly * Updated gm to use flags when calling Hermes * Added missing flags to e2e test for 'query client state' command * Fixed flag errors in e2e tests and removed conflicting short flag. * Removed all short flags and updated CLI commands comments * Removed forgotten short flags. * Updated Hermes guide with flags instead of positional arguments * Updated script and comment with new long flags for Hermes * Completed 'tx raw upgrade-' commands guide page. Updated Testing client upgrade guide page * Added changelog entry * Added example unit-tests to the 'keys add' command * Added value names to parameters and removed cli parsing unit-tests * Cargo fmt changes * Updated flags in order to reflect ADR 010 * Updated guide to reflect flag changes from ADR 010 * Updated gm script and e2e tests to match flag changes from ADR 010 * Fixed ADR 010 typo * Remove short flags from `key` error messages * Fix inconsistent error messaging in channel.rs * Fix more inconsistent error messaging * Run cargo fmt * Fix commands that use deprecated `-c` flag * Added the query transfer subcommand in docs. * Scrub query clients * Scrub query client state * Removed excessive debug line * Disabled filtering for query channels CLI * Added aliases for 'connection', 'channel' and 'sequence' flags * Fixed error in CLI replacing HeightQuery with QueryHeight * Fix cargo fmt * Changed 'version' to 'channel-version' for 'hermes create channel' command * Separated 'hermes create channel' help into short and long message * Updated 'hermes listen' so that '--events' flag can take multiple values * Updated guide with new '--events' flag for 'hermes listen' * Start all arguments help text with an uppercase letter, to match Clap's help for autogenerated arguments (eg. --help) * Update guide to account for argument help text starting with an uppercase letter * Add shortened aliases for `tx raw` commands Co-authored-by: Sean Chen <seanchen11235@gmail.com> Co-authored-by: Adi Seredinschi <adi@informal.systems> Co-authored-by: Romain Ruetschi <romain@informal.systems>

ljoss17 added 8 commits June 7, 2022 11:31

Updated CLI commands to take flags everywhere and updated e2e tests a…

51852ae

…ccordingly

Updated gm to use flags when calling Hermes

21580d3

Added missing flags to e2e test for 'query client state' command

09c157f

Fixed flag errors in e2e tests and removed conflicting short flag.

612a707

Removed all short flags and updated CLI commands comments

88474db

Removed forgotten short flags.

a76599f

Updated Hermes guide with flags instead of positional arguments

bf60ca1

Updated script and comment with new long flags for Hermes

b9983ef

ljoss17 added 2 commits June 10, 2022 15:14

Completed 'tx raw upgrade-' commands guide page. Updated Testing clie…

eb13cd0

…nt upgrade guide page

Added changelog entry

7a25573

ljoss17 marked this pull request as ready for review June 10, 2022 13:46

ljoss17 requested review from mzabaluev, soareschen, romac, ancazamfir and seanchen1991 as code owners June 10, 2022 13:46

ljoss17 marked this pull request as draft June 15, 2022 14:57

Added example unit-tests to the 'keys add' command

2e3fff3

ljoss17 mentioned this pull request Jun 16, 2022

ADR for unified approach for CLI arguments for Hermes #2306

Merged

6 tasks

ljoss17 commented Jun 16, 2022

View reviewed changes

ljoss17 added 5 commits June 17, 2022 09:48

Added value names to parameters and removed cli parsing unit-tests

ff84119

Cargo fmt changes

4d35800

Updated flags in order to reflect ADR 010

c7e1b9a

Updated guide to reflect flag changes from ADR 010

c363e04

Updated gm script and e2e tests to match flag changes from ADR 010

9e6d309

ljoss17 marked this pull request as ready for review June 20, 2022 07:25

mzabaluev reviewed Jun 27, 2022

View reviewed changes

Merge branch 'master' into luca_joss/replace_positional_args_with_flags

49b4a41

ljoss17 and others added 10 commits June 28, 2022 12:00

Added aliases for 'connection', 'channel' and 'sequence' flags

840a51c

Fixed error in CLI replacing HeightQuery with QueryHeight

d389f3d

Fix cargo fmt

e06b55a

Changed 'version' to 'channel-version' for 'hermes create channel' co…

fd71131

…mmand

Separated 'hermes create channel' help into short and long message

64e54e2

Updated 'hermes listen' so that '--events' flag can take multiple values

d2ef49c

Updated guide with new '--events' flag for 'hermes listen'

7d8a58e

Start all arguments help text with an uppercase letter, to match Clap…

92cd463

…'s help for autogenerated arguments (eg. --help)

Update guide to account for argument help text starting with an upper…

eff375c

…case letter

Add shortened aliases for tx raw commands

6b2947e

romac approved these changes Jun 28, 2022

View reviewed changes

adizere merged commit 57b8af9 into master Jun 28, 2022

adizere deleted the luca_joss/replace_positional_args_with_flags branch June 28, 2022 13:08

mmsqe added a commit to mmsqe/pystarport that referenced this pull request Jul 27, 2022

avoid use confusing flag after

54bc696

informalsystems/hermes#2275

mmsqe mentioned this pull request Jul 27, 2022

Avoid use confusing flag after ibc-rs#2275 crypto-com/pystarport#66

Merged

yihuang pushed a commit to crypto-com/pystarport that referenced this pull request Jul 28, 2022

avoid use confusing flag after (#66)

537cfbd

informalsystems/hermes#2275

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace positional arguments with flags in CLI #2275

Replace positional arguments with flags in CLI #2275

ljoss17 commented Jun 7, 2022 •

edited by adizere

Loading

plafer commented Jun 8, 2022

ljoss17 commented Jun 9, 2022

hu55a1n1 commented Jun 14, 2022

ljoss17 Jun 15, 2022

ancazamfir Jun 16, 2022

hu55a1n1 Jun 17, 2022

mzabaluev Jun 18, 2022

ljoss17 Jun 20, 2022

mzabaluev commented Jun 18, 2022

romac commented Jun 24, 2022

ljoss17 commented Jun 24, 2022

romac commented Jun 24, 2022

ljoss17 commented Jun 24, 2022

romac commented Jun 24, 2022

ljoss17 commented Jun 24, 2022

seanchen1991 commented Jun 24, 2022

romac commented Jun 27, 2022

mzabaluev left a comment

mzabaluev commented Jun 27, 2022

romac commented Jun 28, 2022

ljoss17 commented Jun 28, 2022

romac left a comment

Replace positional arguments with flags in CLI #2275

Replace positional arguments with flags in CLI #2275

Conversation

ljoss17 commented Jun 7, 2022 • edited by adizere Loading

Description

WIP

PR author checklist:

Reviewer checklist:

plafer commented Jun 8, 2022

ljoss17 commented Jun 9, 2022

hu55a1n1 commented Jun 14, 2022

ljoss17 Jun 15, 2022

Choose a reason for hiding this comment

ancazamfir Jun 16, 2022

Choose a reason for hiding this comment

hu55a1n1 Jun 17, 2022

Choose a reason for hiding this comment

mzabaluev Jun 18, 2022

Choose a reason for hiding this comment

ljoss17 Jun 20, 2022

Choose a reason for hiding this comment

mzabaluev commented Jun 18, 2022

romac commented Jun 24, 2022

ljoss17 commented Jun 24, 2022

romac commented Jun 24, 2022

ljoss17 commented Jun 24, 2022

romac commented Jun 24, 2022

ljoss17 commented Jun 24, 2022

seanchen1991 commented Jun 24, 2022

romac commented Jun 27, 2022

mzabaluev left a comment

Choose a reason for hiding this comment

mzabaluev commented Jun 27, 2022

romac commented Jun 28, 2022

ljoss17 commented Jun 28, 2022

romac left a comment

Choose a reason for hiding this comment

ljoss17 commented Jun 7, 2022 •

edited by adizere

Loading