src: add getopt option parser #1804
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 :] |
| * string begins with a '+'. | ||
| */ | ||
| if (posixly_correct == -1 || optreset) | ||
| posixly_correct = (getenv("POSIXLY_CORRECT") != NULL); |
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 |
| @@ -178,6 +179,7 @@ typedef intptr_t ssize_t; | |||
|
|
|||
| namespace node { | |||
|
|
|||
| NODE_EXTERN extern NodeOptions node_options; | |||
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 --help | ||
| var child = spawn(process.execPath, ['--help']); | ||
| child.stdout.on('data', function(chunk) { | ||
| assert.equal(/Usage/.test(chunk.toString()), true); |
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>
Initial getopt implementation for option parsing. Supersedes #1726.
I split out the actual options to try and make it more maintainable.