-
Notifications
You must be signed in to change notification settings - Fork 136
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
cli help text updates and flags cleanup #1564
Conversation
- verbose flag uses global setting throughout - confirm flag added using shared visitor throughout - path flag added using shared visitor throughout - removes --version flag on root as redundant with subcommand - splits main help's 'Main Commands' into 'Primary Commands' and 'Development Commands' groups - Moves RunE definition into flag struct literals
a267099
to
a3c7a1b
Compare
Codecov ReportBase: 60.58% // Head: 60.61% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1564 +/- ##
==========================================
+ Coverage 60.58% 60.61% +0.03%
==========================================
Files 75 76 +1
Lines 10340 10363 +23
==========================================
+ Hits 6264 6282 +18
+ Misses 3508 3502 -6
- Partials 568 579 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
/lgtm
Great job!
/hold for others
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.
Looks great overall - nice clean up. I just have a small suggestion and one question about the version
values.
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.
/lgtm
Great improvement!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkingland, zroubalik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
Changes
/kind cleanup
Notes
Refactor of root help text
In order to cohere more with kn, the root help text has been reworked to be
quite similar. Subcommands were not (yet) updated. Moving the lone
global flag, verbose, to subcommands assisted in this homogenization
Addition of the verbose flag directly to commands
The complexity added by having a single global (persistent) flag was not
worth the benefit of being able to elide defining it on each command.
Furthermore, when used as a plugin (to kn for example) it is easy to be
confused which set of global commands apply where. It's clearly simpler
to just
addVerbose(cmd)
right along with all the other flags.Renaming setXFlag functions to addXFlag
setX usually implies that the object with context to the accessor will be
mutaed (user.setName("bob"), etc). Whereas addX resolves this ambiguity
by implying the passed argument will be mutated.
Removing the redundant --version flag
Both to be more coherent with kn, and because it seems to be the emergent
idiom for Go apps, the builtin --version flag was removed in favor of
the extant subcommand.
Ensuring --confirm and --path flags use accessor
As stated above, there is not enough flag-usage overlap between commands
to make global ("persistent") flags correct, but there are three which
are used enough to make a helper function useful: verbose, path and confirm.
These were added where missing throughout.
Creation of the Primary Commands and Development Commands groups
It is important to list the primary CRUD operations as the primary commands,
so that new users understand the high-level lifecycle of a function.
This creates that group, moving development/helper commands such as build,
run and invoke to their own group.
Release Note