Skip to content
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

feat: add --version option #66

Merged
merged 3 commits into from
Nov 9, 2017

Conversation

GantMan
Copy link
Contributor

@GantMan GantMan commented Nov 8, 2017

closes #65

@GantMan
Copy link
Contributor Author

GantMan commented Nov 8, 2017

image

@machour machour changed the title add version option feat: add version option Nov 8, 2017
Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🎉

Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GantMan Had a doubt while rechecking and noticing promptForCommand being hijacked.
I think this could be done in a nicer way, see comments ;)

cli.js Outdated
alias: 'v',
desc: 'Print installed version',
type: 'string'
})
.default('contributors', [])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be the standard way to display version information using yargs:
https://github.com/yargs/yargs/blob/master/docs/api.md#version

Reading the docs, I think that this should be enough:

  .alias('v', 'version')
  .version()

Also, could you move this block right under alias('h', 'help')? It would make more sense ;)

cli.js Outdated
console.log(require('./package.json').version);
process.exit(0);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be needed anymore

@GantMan
Copy link
Contributor Author

GantMan commented Nov 9, 2017

@machour - you're right!

I'm not sure how I missed that! updated 👍

Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries @GantMan :)
Just a final thing to take care of and this baby can be merged in

cli.js Outdated
@@ -29,6 +29,8 @@ var argv = yargs
.boolean('commit')
.default('files', ['README.md'])
.default('contributorsPerLine', 7)
.alias('v', 'version')
.version()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now if we can have these two lines moved up under help, it would be amazing 😅

@GantMan
Copy link
Contributor Author

GantMan commented Nov 9, 2017

👍 @machour

@machour
Copy link
Collaborator

machour commented Nov 9, 2017

Thank you so much for baring with me matey!

@machour machour changed the title feat: add version option feat: add --version option Nov 9, 2017
@machour machour merged commit 9f72f9a into all-contributors:master Nov 9, 2017
@GantMan
Copy link
Contributor Author

GantMan commented Nov 9, 2017

Quality over quantity any day!

@GantMan GantMan deleted the feature/version branch November 9, 2017 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add version flag
2 participants