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

Changed option parser for cli #399

Merged
merged 14 commits into from
Mar 10, 2017
Merged

Changed option parser for cli #399

merged 14 commits into from
Mar 10, 2017

Conversation

jankowiakmaria
Copy link
Collaborator

@jankowiakmaria jankowiakmaria commented Mar 9, 2017

resolves #135

Differences:

  • removed version command (it has version option instead)
  • no description of positional arguments
  • added examples for each command (which should mitigate ^^)
  • positional argument !== option (in nomnom both types could be used interchangeably)
  • producing completion script is done differently (don't need additional dependencies - yargs provides completion script out of the box) - so wiki needs to be updated

Question - should I shrinkwrap or is it done during publishing/at some other point of the process?


dev: {
help: 'Runs a local oc test registry in order to develop and test components',
cmd: 'dev <dirName> [port] [baseUrl]', //should be dirPath imho
Copy link
Contributor

Choose a reason for hiding this comment

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

agree, it should be dirPath

hotReloading: {
boolean: true,
description: 'Enables hot reloading. Note: when hot reloading is set to true, each request to the component will make the registry to create a new instance for the javascript closures to be loaded, while when false the instance will be recycled between components executions',
default: true
Copy link
Member

Choose a reason for hiding this comment

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

+port param here, +baseUrl?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I've mentioned in PR comment - we can either have a positional argument or an option - yargs is really good but it offers different set of features than nomnom (it currently doesn't support description of positional arguments - I've seen workarounds to support it but they're not great)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I totally forgot to read your first PR description. This is fine, sorry :)

},

init: {
help: 'Creates a new empty component in the current folder',
cmd: 'init <componentName>',
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be init <componentName> [templateType] ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as above - if you'd like to I can walk you through differences (I've tried to include everything in the PR comment).

Copy link
Member

Choose a reason for hiding this comment

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

templateType used to be a positional arg https://github.com/opentable/oc/wiki/Cli#init so I think I would like to keep it as is (here you are basically changing it to be an option)

Copy link
Collaborator Author

@jankowiakmaria jankowiakmaria Mar 10, 2017

Choose a reason for hiding this comment

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

fine for me :) will change it
in nomnom you could use it as either of them [positional argument/option] - it would work anyway.

@@ -89,6 +87,7 @@
"uglify-js": "2.6.4",
"underscore": "1.8.3",
"watch": "0.19.1",
"webpack": "2.2.0"
"webpack": "2.2.0",
"yargs": "^6.6.0"
Copy link
Member

Choose a reason for hiding this comment

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

let's pair on the shrinkwrap

Copy link
Member

Choose a reason for hiding this comment

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

Done

@matteofigus
Copy link
Member

@jankowiakmaria ok all good, only thing is on the oc init

@matteofigus matteofigus merged commit 5090c6e into master Mar 10, 2017
@matteofigus matteofigus deleted the cli branch March 10, 2017 13:57
@matteofigus
Copy link
Member

Amazing! Are you able to update the wiki @jankowiakmaria ?

@jankowiakmaria
Copy link
Collaborator Author

yep, sure :)

@jankowiakmaria
Copy link
Collaborator Author

@matteofigus updated https://github.com/opentable/oc/wiki/Cli

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.

Change option parser for cli
3 participants