src: add getopt option parser#1804
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mscdex commented May 27, 2015
It would be nice if we could generate the help text from the defined options. That would help avoid duplication. |
evanlucas commented May 27, 2015
I think that's definitely doable. I'll see if I can add that tonight. |
evanlucasforce-pushed the getopt branch 5 times, most recently from dd59140 to b06ff7bCompareevanlucas commented May 27, 2015
Ok, help is automatically generated now too :] |
deps/getopt/getopt_long.c Outdated
There was a problem hiding this comment.
I suggest you remove this code. I don't think many people would expect POSIXLY_CORRECT to affect the iojs binary.
evanlucasforce-pushed the getopt branch 2 times, most recently from c78d8f8 to 65a192eCompare
brendanashworth added the c++ evanlucas commented May 28, 2015
Switched to use a global variable |
src/node.h Outdated
There was a problem hiding this comment.
For the love of $DEITY, please don't make this public.
bnoordhuis commented May 29, 2015
It's probably a good idea to add some option parsing regression tests. |
evanlucas commented May 29, 2015
Ok, made requested changes and added tests for options that are not currently being tested |
test/parallel/test-cli-help.js Outdated
There was a problem hiding this comment.
This is not 100% sound, the output can get split over multiple chunks. Can I suggest you use spawnSync instead?
EDIT: Or execFile, like you do below.
There was a problem hiding this comment.
sure, will update that now. Want me to do that on all of these tests?
bnoordhuis commented May 29, 2015
LGTM with nits and assuming the CI is happy. |
evanlucas commented May 29, 2015
evanlucasforce-pushed the getopt branch 3 times, most recently from a7752f6 to 68357e3Compareevanlucas commented May 29, 2015
Ok, working on SmartOS support right now. Will run another CI once I get that done. |
evanlucasforce-pushed the getopt branch 2 times, most recently from 3019a34 to eadc181Compareevanlucas commented May 30, 2015
Ok, final CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/736/ Last one had failures related to #1837. |
evanlucas commented Jun 1, 2015
Does this need to wait until 3.0? |
bnoordhuis commented Jun 1, 2015
If there are no user-visible changes, it can go into a patch release. |
evanlucas commented Jun 1, 2015
The actual help message that is printed is slightly different since it is automatically generated based on the options. |
Options have been moved into the NodeOptions class. A new global, node_options now exists and is used to access the options after the command line arguments have been parsed. PR-URL: nodejs#1804 Reviewed-By: Ben Noordhuis <[email protected]>
evanlucas commented Jun 1, 2015
Landed in c0e7bf2. Thanks for the multiple reviews @bnoordhuis |
jbergstroem commented Jun 1, 2015
When this lands again, could we possibly look at expanding the cctest suite? |
evanlucas commented Jun 1, 2015
sure |
Options have been moved into the NodeOptions class. A new global, node_options now exists and is used to access the options after the command line arguments have been parsed. PR-URL: nodejs/node#1804 Reviewed-By: Ben Noordhuis <[email protected]>
5 participants
Initial getopt implementation for option parsing. Supersedes #1726.
I split out the actual options to try and make it more maintainable.