-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Bugfix help command #3706
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
Bugfix help command #3706
Conversation
src/cli/commands/help.js
Outdated
@@ -41,11 +43,14 @@ export function run(config: Config, reporter: Reporter, commander: Object, args: | |||
const getDocsLink = name => `${constants.YARN_DOCS}${name || ''}`; | |||
console.log(' Commands:\n'); | |||
for (const name of Object.keys(commands).sort(sortAlpha)) { | |||
if (commands[name].useless) { | |||
if (commands[name].useless || unsupportedAliases[name] || Object.values(aliases).includes(name)) { |
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.
Not sure the Object.values().includes()
construct will work on Node 4 :/
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 catch. I think the proper support for Array.prototype.includes came with Node 6. I will update the PR.
src/cli/commands/help.js
Outdated
|
||
console.log(` - ${hyphenate(name)}`); | ||
if (aliases[name]) { | ||
console.log(` - ${hyphenate(name)} alias: ${aliases[name]}`); |
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.
Could we just print - {name} / {alias1} / {alias2} / ...
to be less verbose?
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 agree, that will get the job done and would look cleaner as well. I will update the PR accordingly.
@voxsim I'd be ok with that - @arslanarshad31 what do you think? Would you have time to do it? |
Ok, let's merge this as it is obviously an improvement over current situation. |
@voxsim @arcanis I think it should be fine to get rid of the unsupported aliases, I don't see them providing a very significant functionality as well and issues created because of them only slow down the development process. |
Go ahead
…On 6 July 2017 at 21:27, Arslan Arshad ***@***.***> wrote:
@voxsim <https://github.com/voxsim> @arcanis <https://github.com/arcanis>
I think it should be fine to get rid of the unsupported aliases, I don't
see them providing a very significant functionality as well and issues
created because of them only slow down the development process.
Also, yes I can open a new PR in the coming days to nuke these unsupported
aliases.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#3706 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACBdWOP8vLkNZy69iX8Jm_VDIkLEKOA0ks5sLbOWgaJpZM4ODM8v>
.
|
Summary - Bug Fix
This PR fixes #3559
The code removes unsupported aliases from the output list of help command and also prints out the supported alias (if there is any) for the command in the same row. For clarification, please have a look at the command
generate-lock-entry
andupgrade-interactive
in the output below, you should be able to see the alias of the commands in the same line.Output with this fix