Re: Make 'traceur' cleanup process.argv and setup the node.js module so that relative require works. (issue 7552046)

usrb...@yahoo.com

unread,

Mar 28, 2013, 11:48:24 AM3/28/13

to edy....@gmail.com, a...@chromium.org, traceur-comp...@googlegroups.com, re...@codereview-hr.appspotmail.com

By the way, reviewers don't get notifications unless you do a
"Publish+Mail Comments" -- even if it's just an empty one.

Most of the final issues from now on will probably be about coding
style and convention. Sometimes it seems silly, but there's a certain
discipline in coding in a style different from your usual one (you may
see my preferred style peeking out occasionally).

arv has the final say on this one, but I'll try to cover some of the
formatting and coding style issues I know he'll probably bring up
(because I've had to fix them in my own code reviews).

----

The code I suggested below is untested, but it should be enough to get
the general idea.

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js
File src/node/command.js (right):

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode70
src/node/command.js:70: // without the file name and anything past that.
Traceur coding style uses JSDoc for most functions (I notice that we're
not
always consistent about it, but still). Since this is your first patch,
here's a template:

/**
* HACK: Process arguments so that in interpret mode, commander.js only
sees the
* flags, without the file name and anything past that. If an invalid
flag is
* encountered, only the arguments up to the invalid flag are passed on.
This is
* necessary in order to reliably trigger commander.js's error
reporting.
* @param {Array.<string>} argv
* @return {Array.<string>}
*/

I did a quick update to the comment to match the current expected
functionality. Reword or change however you want.

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode77
src/node/command.js:77: if (arg == '--' || arg == '--out')
traceur coding style uses '===' and '!=='.

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode85
src/node/command.js:85: arg = argv[i+1];
argv[i + 1];
spaces around operators.

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode93
src/node/command.js:93: argv.splice(i, 0, '--');
I had a closer look through commander.js and I found out why invalid
flags weren't being flagged (so to speak). Turns out it's supposed to be
a feature.

If there are any extra pieces that aren't flag-like, then it doesn't
consider the invalid flags as errors. So the only way to mark them as
errors is to filter out the extra pieces that aren't flag-like. And
even then, you're only going to get an error for the first invalid flag.

'gcc' is much nicer, and flags all invalid flags. By default, it
doesn't assume invalid flag take args, either.

In that case, you might as well not bother going to the trouble of doing
the filter at all, and just return the first invalid arg.

// If it's an invalid flag, we want it to show up as an error. Due to
// a quirk in commander.js, we only get an error message for the first
// invalid flag, so just return that one and don't bother finding the
// rest.
if (arg[0] === '-')
argv.splice(i + 1);
else
argv.splice(i, 0, '--');

To be honest, this library has made some interesting design decisions.

'processArguments' should probably be marked 'HACK' to make these things
easier to find for later cleanup.

----

I'm still trying to figure out what would be the cleanest way to fix
these bugs upstream. This is what I'm currently considering:

- An option to make unknown flags that occur before the first extra
regular arg into errors, even though I really think this should always
be the case. So maybe I won't even make it an option, and see if that
flies.
- The ability to set a callback for when the first regular arg is
encountered.
- Have the ability for option callbacks to modify later arg parsing.
Specifically, the ability to force the rest of the args to be pushed
verbatim into flags.args

----

Just realized. There is a bug in the code. Or rather a bug in the
workaround
for commander.js's bugs.

## bug, should error out on '--invalid-arg'.
./traceur --out file.out.js file.js --invalid-arg
./traceur --out file.out.js --invalid-arg file1.js file2.js

Honestly, this is getting hackier and hackier all the time, but we've
gone
this far already, so it's not too much more code:

if (arg === '--out') {
i++;
compile = true;
}

...

// HACK: Argh!!!!! HACKHACKHACK
if (arg[0] === '-') {
argv.splice(i + 1);
break;
} else if (!compile) {
argv.splice(i, 0, '--');
break;
}

https://codereview.appspot.com/7552046/