-
Notifications
You must be signed in to change notification settings - Fork 24
fix: include the top level app command with sub commands in help page examples #93
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
Conversation
zimeg
left a comment
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.
📝 One note for the observant and kind reviewers-
| }, | ||
| PostRunE: func(cmd *cobra.Command, args []string) error { | ||
| ctx := cmd.Context() | ||
| // DEPRECATED(semver:major): remove the "workspace" alias |
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.
📣 Wanting to call this change out! This matches deprecations that we might hope to search for in the next set of breaking changes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
- Coverage 63.20% 63.19% -0.01%
==========================================
Files 210 210
Lines 22194 22195 +1
==========================================
- Hits 14028 14027 -1
- Misses 7081 7084 +3
+ Partials 1085 1084 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
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 catching this @zimeg!
🤔 It makes me wonder if we should display the full command for the other as well: app install, app uninstall, and app delete. For someone looking at the help info for app, it'd odd to see examples that aren't "using" the app command.
|
@mwbrooks And thanks for the kind review 🙏 ✨ I was also wondering that and am so open to making that update, but was wanting to keep this PR scoped to just a fix to start. I found this confusing myself though... FWIW the The mismatch does seem so strange here. Let me know if you're also thinking about the long form examples read best: I'm so open to updating just these subcommands or all other commands now too to avoid later alias breakings that could perhaps happen 👾 |
|
@zimeg I agree, we should keep this PR scoped to the app commands and merge it It would make sense to open a new PR that updates all commands to be consistent. I suppose that means we need to decide what standard to use. Off the top of my head, I feel the subcommand examples should be prefixed with the subcommand (e.g. |
|
@mwbrooks Wonderful ideas! I am a big fan 👾
This seems best with a somewhat scoped GLOBAL ALIASES section appearing above other sections in For now let's merge this PR with changes to the other |
|
Edit: Oh I sent that too soon, but do let me know if repeating the global aliases is seeming ideal! We can also discuss this point more in a follow up. I took a note to follow up on this 🤖 |
Summary
This PR includes the
appportion of theapp linkandapp listcommands to avoid aliased errors.Preview
Requirements