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

cmd: limit global flags displayed on cli help output #5077

Merged
merged 6 commits into from Oct 11, 2023

Conversation

sreuland
Copy link
Contributor

@sreuland sreuland commented Oct 6, 2023

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

  • Limit which global flags will display on -h cli output to when it's only relevant to the command's general roles that it may be performing 'ingestion', 'db update tool' or 'api server'.
  • Don't show global flags on help output when the most specific sub-command still has child sub-commands, limit help output to just list of available sub-commands.

Why

The global flags are a fixed set of parameters and get displayed as a single large block on the console help output for any help request regardless of the level of horizon command. Since it's a fixed list, many of the global parameters tend to not be relevant to the context of command being requested by help and therefore can be somewhat mis-leading.

Closes #5022

Notes for review:

Start here, the solution overrides the cobra HelpFunc:
https://github.com/stellar/go/pull/5077/files#diff-7ad09d8c6e6a91967287e26e0343f0262ed02aa9fab85a0741cf665eeab514efR52

Some screen shots of help output from this PR on top, and current horizon on bottom.


`stellar-horizon -h`.

Screen Shot 2023-10-05 at 8 40 06 PM



`stellar-horizon ingest -h`.

Screen Shot 2023-10-05 at 8 41 50 PM




Screen shot of help output from this PR on left, and current horizon on right:
stellar-horizon db reingest range -h.

Screen Shot 2023-10-05 at 8 50 39 PM

@sreuland sreuland requested a review from a team October 6, 2023 16:42
Copy link
Contributor

@aditya1702 aditya1702 left a comment

Choose a reason for hiding this comment

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

LGTM. Just need to resolve the merge conflicts.

Copy link
Contributor

@urvisavla urvisavla left a comment

Choose a reason for hiding this comment

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

The changes look great! The only thing I noticed is that horizon db reingest displays all global flags, whereas horizon db reingest -h does not. I believe horizon db reingest cannot be used on its own without specifying a range so I expected the output of horizon db reingest to same as horizon db reingest -h

@sreuland
Copy link
Contributor Author

sreuland commented Oct 9, 2023

The changes look great! The only thing I noticed is that horizon db reingest displays all global flags, whereas horizon db reingest -h does not. I believe horizon db reingest cannot be used on its own without specifying a range so I expected the output of horizon db reingest to same as horizon db reingest -h

good catch, yeah, I think cobra is routing through a 'usage' output function there, so the hooked 'help' didn't get a chance to run, I'll look into it further to see if can capture that path, thanks!

@sreuland
Copy link
Contributor Author

The changes look great! The only thing I noticed is that horizon db reingest displays all global flags, whereas horizon db reingest -h does not. I believe horizon db reingest cannot be used on its own without specifying a range so I expected the output of horizon db reingest to same as horizon db reingest -h

@urvisavla , fortunately, cobra allowed overriding the usage output in same way as help output, so i was able to apply the same global flags behavior in this case of usage - deca1cc

@sreuland sreuland merged commit 3ce1cea into stellar:master Oct 11, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

services/horizon/flags: refined cli help content
3 participants