Skip to content

(#80) - Use nomnom for argument parsing#80

Closed
marten-de-vries wants to merge 1 commit intomasterfrom
nomnom
Closed

(#80) - Use nomnom for argument parsing#80
marten-de-vries wants to merge 1 commit intomasterfrom
nomnom

Conversation

@marten-de-vries
Copy link
Contributor

Why the switch

  1. optimist has been deprecated
  2. As part of Get config defaults from config file #72 I'm likely to add new options soon (ones for cors e.g.) Having usage info in 2 places instead of three is handy then.
  3. As part of Get config defaults from config file #72 this project will become more complicated. Things like binding to a new port/ip while running need to be supported, as well as changing the PouchDB object while running. Less global variables and validation of these variables later on will definitely help then. Nomnom helps in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Splitting the usage strings would become very messy, even more since adding newlines would stop 'wordwrap' from working. I decided to just turn length checking off instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the usage strings, you can just precede your super-long strings with:

/* jshint maxlen:false */

and then afterwards:

/* jshint maxlen:80 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, that's the better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like jshint/jshint#1313 prevents me from doing that (my best guess is that it sees the node option as being inside an anonymous function). Wrapping the argument parsing inside a function and disabling maxlen for that function only works, so I used that as a workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay, good to know

@nolanlawson
Copy link
Contributor

+1 except for that nit about maxlen. I've been using yargs recently due to @calvinmetcalf's recommendatin, but nomnom looks nice too.

@marten-de-vries
Copy link
Contributor Author

yargs is the other one I looked into, it's the more direct successor to optimist but I actually liked nomnom's api better.

marten-de-vries added a commit that referenced this pull request Dec 18, 2014
@marten-de-vries
Copy link
Contributor Author

Fixed all the things commented upon, and merged: 4de5ced

Thanks for the elaborate reviews!

@nolanlawson
Copy link
Contributor

Yeah man, no prob. Everything you've been doing here has been really cool. :)

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.

2 participants