Skip to content
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

Disable clap-provided -h flags on two subcommands #1739

Closed
wants to merge 1 commit into from

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Jan 4, 2022

Description

The conflicting flags were overlooked during the original clap porting in #1576, resulting in panics when query client consensus and query client connections commands are invoked.

This is an alternative to changes made in 60829af, but with no CLI breakage.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

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

These were overlooked during the original clap porting.
@mzabaluev mzabaluev added A: bug Admin: something isn't working I: CLI Internal: related to the relayer's CLI labels Jan 4, 2022
@mzabaluev
Copy link
Contributor Author

Re tests: evidently there are no integration tests to exercise these subcommands in CI. How difficult it would be to add the tests, considering that the only way to invoke these commands in a do-nothing mode is being removed by this PR?

@romac
Copy link
Member

romac commented Jan 4, 2022

That's nice alternative, but if we go this way how about doing so for all commands? This way -h will never mean --help for one command and --height for another.

@mzabaluev
Copy link
Contributor Author

@romac Unfortunately, the option also disables the long --help flags for these select subcommands.
So it's a worse solution to your renaming all height-related short flags to -H, which I assume can be an acceptable breakage for the 0.10.0 release.

@romac
Copy link
Member

romac commented Jan 4, 2022

Oh I didn't realize it would suppress the --help flag as well, then yeah let's stick with -H flags and incur some breakage.

@romac
Copy link
Member

romac commented Jan 4, 2022

Let's close this in favor of 60829af in #1712

@romac romac closed this Jan 4, 2022
@romac romac deleted the mikhail/deconflict-h-flags branch January 4, 2022 14:18
@mzabaluev
Copy link
Contributor Author

Superseded by #1743

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Admin: something isn't working I: CLI Internal: related to the relayer's CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants