-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: improve inline documentation #172
Conversation
apps/cli/src/command/helper.ts
Outdated
constructor(name: string, summary?: string, description?: string) { | ||
super(name); | ||
|
||
if (summary) this.summary(summary); |
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.
This adds a summary for describing the subcommand in the global help while preserving the longer description for the subcommand-specific help.
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.
There are some indentation issues, and I recommend you install the Prettier plugin for VSCode, which will format any changes you make following the code formatting configuration in our repository.
Please turn on the Format on Save
option for the VSCode project workspace, or use the Format Document
command manually.
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 have no problem with correctness on TypeScript.
The changes on the inline docs look good to me, but still need to be checked by other reviewers.
@@ -158,14 +158,16 @@ export const DiffResourceTask = ( | |||
|
|||
export const DiffCommand = new BackendCommand<DiffOptions>( | |||
'diff', | |||
'Show the difference between local and backend configurations', | |||
'show differences between the local and the backend configurations', | |||
'Compare the configuration in the specified file(s) with the backend configuration', |
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.
Only curious to know why the first sentence needs to be lower cased
I noticed all similar instances are changed to lower case
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.
By default the library we use, commander, uses lower case when showing the summary of a command/flag, like the in-built help command/version flag.
I just followed the convention.
The alternative was to use custom help and version commands which seemed unnecessary.
I also don't have strong opinions on choosing one over the other.
Description
Makes improvements to the inline documentation of subcommands so that they are consistent.
I will also be improving the examples shown to cover typical use cases and making some other UX improvements. I realized it might take some time and become a bigger PR so this is a small PR to make some initial changes.
Checklist