-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ADR for unified approach for CLI arguments for Hermes #2306
ADR for unified approach for CLI arguments for Hermes #2306
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up @ljoss17 . I left a few minor comments.
* Any parameter ending with `_id` should have the `_id` removed from the flag to shorten it. For example the flag for `chain_id` should only be `chain`. | ||
* Flags which can be shorten and still be meaninful should be shorten. This is done for `connection`, `channel` and `sequence`, which become respectively `conn`, `chan` and `seq`. | ||
* In cases where there are source and destination parameters for a same object, the flags start with the prefix `--src-` and `--dst-`. | ||
* In cases where there are undirectional parameters for a same object, the flags end with the suffix `-a` and `-b`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are "unidirectional" parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we have src_chain
but chain_a
. In the code we use src_chain
and a_chain
. The qualifier should be either prefix or suffix but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My choice of word is confusing sorry. My interpretation of nondirectional parameters are the ones where using parameters --a A
and --b B
will result the same as --a B
and --b A
.
Let's use create connection
as an example. There are two chains ibc-0
and ibc-1
. There will have be an internal difference between --chain-a ibc-0
and --chain-b ibc-1
and --chain-a ibc-1
and --chain-b ibc-0
, which is the chain that initialises the handshake. But the created connection is the same.
This is the reason why src
and dst
seemed confusing if used for all the cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My choice of word is confusing sorry. My interpretation of nondirectional parameters are the ones where using parameters --a A and --b B will result the same as --a B and --b A.
But the result is the same. I propose we do not use unidirectional. We can say that A and B identify entities (chains, clients, connections, ports) on the two ends of a connection or channel. We can just mention that OpenInit
is sent to chain A.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we have
src_chain
butchain_a
. In the code we usesrc_chain
anda_chain
. The qualifier should be either prefix or suffix but not both.
I'm a bit confused by this as there is a mix of both chain_a
and a_chain
in the code. From a quick search I only see a_chain
in the relayer
create. But chain_a
is in both the relayer
and relayer-cli
crates. If only prefix or suffix should be used, I find the prefix better, mostly for source and destination.
My choice of word is confusing sorry. My interpretation of nondirectional parameters are the ones where using parameters --a A and --b B will result the same as --a B and --b A.
But the result is the same. I propose we do not use unidirectional. We can say that A and B identify entities (chains, clients, connections, ports) on the two ends of a connection or channel. We can just mention that
OpenInit
is sent to chain A.
I will change my phrasing to: "In cases where there are two separate parameters for a same object type" would that seem better ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good! @ljoss17 could you please make the changes and we can see how they read? I think we are pretty close to merging this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put them to suffixes, let me know if this is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i read the doc again and not super happy with how chain-host
and chain-reference
read. The latter for example reads "the reference of the chain". My order of preference now is:
a-chain
,host-chain
,reference-chain
chain-a
,host-chain
,reference-chain
...even though i think it's not consistent i now prefer it slightly over 3.chain-a
,chain-host
,chain-reference
@adizere, @mircea-c et al. wdyt?
I will let you all decide, in the end I am ok with any of the above and will be much better than what we have today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am partial to the first option. Only thing I don't like is the suffix for the unidirectional
. Could just be chain
without a suffix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First option sounds good also to me!
Only thing I don't like is the suffix for the unidirectional. Could just be chain without a suffix
Not sure what you mean.
@mircea-c any thoughts on this proposed cli? |
I've been tagged so I'll stick my nose in here 😁 Changes look good. Very happy to see an effort to get rid of positional parameters. Left some comments in the previous reviews. |
Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
…from prefixes to suffixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @ljoss17!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thanks for doing this @ljoss17, I'm feeling much better about v1.0 now 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work Luca!!
…s#2306) * Added ADR for unified CLI arguments for Hermes * Update docs/architecture/adr-010-unified-cli-arguments-hermes.md Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Update docs/architecture/adr-010-unified-cli-arguments-hermes.md Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Update docs/architecture/adr-010-unified-cli-arguments-hermes.md Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Improved coherence in parameter names and improved section separation. * Added precision to flag name and parameter name for 'query clients' * Fixed parameter name * Replaced 'src' and 'dst' by 'host' and 'reference' * Improvements following PR reviews * Reordered queries and added annotation for CLI optional flags not yet implemented * Moved all specifiers for 'chain', 'connection', 'channel' and 'port' from prefixes to suffixes * Updated client flag for 'update client' and 'upgrade client' commands * Updated 'chain', 'connection', 'channel' and 'port' specifiers to be prefixes * Added changelog entry and updated ADR with small fixes Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Closes: #XXX
Description
Rendered link:
https://github.com/informalsystems/ibc-rs/blob/luca_joss/adr_cli_replace_pos_args_with_flags/docs/architecture/adr-010-unified-cli-arguments-hermes.md
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.