-
Notifications
You must be signed in to change notification settings - Fork 30
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
Command options should also show up when using --help #26
Conversation
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.
good change, we should definitely be doing this. A couple things to address but it's mostly ready.
src/command.ts
Outdated
let options | ||
if (a.options) { | ||
options = `Can be one of: ${a.options.join(', ')}` | ||
} | ||
let description = a.description || '' | ||
if (a.default) description = `[default: ${a.default}] ${description}` |
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.
can you instead do:
if (a.options) description = `(${a.options.join('|')}) ${description}`
This will better match how we show flag options.
Also, passing in a 3-length array to the list is likely not going to work in all cases. That behavior I plan to remove as we're not actually using it anyways I don't think.
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.
Updated.
src/commands/options.ts
Outdated
|
||
// import Help from '..' | ||
|
||
export default class OptionsCommand extends Command { |
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 would create a new command for all the CLIs which we definitely do not want. Inline the class like we do here: https://github.com/oclif/plugin-help/blob/master/test/command.test.ts#L116
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.
Makes sense to me now :D I updated the tests
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
========================================
+ Coverage 73.59% 74.1% +0.5%
========================================
Files 7 7
Lines 250 251 +1
Branches 76 77 +1
========================================
+ Hits 184 186 +2
Misses 27 27
+ Partials 39 38 -1
Continue to review full report at Codecov.
|
<a name="2.0.5"></a> ## [2.0.5](v2.0.4...v2.0.5) (2018-06-06) ### Bug Fixes * show argument options in help ([#26](#26)) ([0c8d1bf](0c8d1bf))
🎉 This PR is included in version 2.0.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Martin Wentzel <w***@s***.de>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated. |
As I kind of need the feature described in oclif/oclif#55 I just went ahead and started.
I have two questions to be able to further continue:
options.test.ts
insidetest/commands
. Do I simply need to adapt theexpect(ctx.stdout).to.equal(....)
to match the desired output?args
function, but even after ayarn build
it does not show the desired output. What am I missing?