Conversation
|
It would be nice if we could generate the help text from the defined options. That would help avoid duplication. |
|
I think that's definitely doable. I'll see if I can add that tonight. |
evanlucas
force-pushed
the
getopt
branch
5 times, most recently
from
dd59140 to
b06ff7b
Compare
|
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.
evanlucas
force-pushed
the
getopt
branch
2 times, most recently
from
c78d8f8 to
65a192e
Compare
brendanashworth
added
the
c++
|
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.
|
It's probably a good idea to add some option parsing regression tests. |
|
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?
|
LGTM with nits and assuming the CI is happy. |
evanlucas
force-pushed
the
getopt
branch
3 times, most recently
from
a7752f6 to
68357e3
Compare
|
Ok, working on SmartOS support right now. Will run another CI once I get that done. |
evanlucas
force-pushed
the
getopt
branch
2 times, most recently
from
3019a34 to
eadc181
Compare
|
Ok, final CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/736/ Last one had failures related to #1837. |
|
Does this need to wait until 3.0? |
|
If there are no user-visible changes, it can go into a patch release. |
|
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 <info@bnoordhuis.nl>
|
Landed in c0e7bf2. Thanks for the multiple reviews @bnoordhuis |
|
When this lands again, could we possibly look at expanding the cctest suite? |
|
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 <info@bnoordhuis.nl>
5 participants
Initial getopt implementation for option parsing. Supersedes #1726.
I split out the actual options to try and make it more maintainable.