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

docs: adding new cli option #747

Merged
merged 2 commits into from Jan 11, 2017

Conversation

Projects
None yet
5 participants
@saintsebastian
Copy link
Collaborator

commented Jan 11, 2017

Added information on adding new cli options and some general CLI info to Contributing Guide.

@coveralls

This comment has been minimized.

Copy link

commented Jan 11, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1882ee3 on saintsebastian:docs-739 into 688d5cf on mozilla:master.

@kumar303
Copy link
Member

left a comment

Thanks for getting this started!

requiresArg: true,
type: 'boolean',
coerce: nameOfCoerceFunction,
},

This comment has been minimized.

Copy link
@kumar303

kumar303 Jan 11, 2017

Member

This looks good but it's a little bit abstract which will make it hard for someone to understand. If it were me, I'd be wondering what is new-option and what is alias? I would suggest trying to make it a bit more realistic, like:

'file-path': {
  describe: 'An absolute file path.',
  alias: ['fp'],
  demand: false,
  requiresArg: true,
  coerce: nameOfCoerceFunction,
},

That way you can refer to it as --file-path=./path/to/file which looks more like a realistic option. You can talk about --fp as the alias which makes a bit more sense. I also removed the boolean type because requiresArg: true contradicts that.

@coveralls

This comment has been minimized.

Copy link

commented Jan 11, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling b24d239 on saintsebastian:docs-739 into 688d5cf on mozilla:master.

@kumar303
Copy link
Member

left a comment

This is great, thanks. This will be really helpful. I might edit this after it lands just to make it a bit more clear for newbies.

@kumar303 kumar303 merged commit d18a73a into mozilla:master Jan 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@saintsebastian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2017

@kumar303 Thanks! I tried to recreate how I've learned doing that, will try add better examples next time.

@rpl

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

@saintsebastian 👍 Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.