Provide an API to pass parameters which resemble options#792
Conversation
This adds the special option string `--`, which means "no options". This can be passed if the first parameter looks like an option (starts with a `-` followed by 1+ letters). Fixes #778
freitagbr
left a comment
There was a problem hiding this comment.
I'm not sure this implementation will work in all cases. Let's think of a different way to handle this.
| If you need to pass a parameter that looks like an option, you can do so like: | ||
|
|
||
| ```js | ||
| shell.grep('--', '-v', 'path/to/file'); // Search for "-v", no grep options |
There was a problem hiding this comment.
Hmmm... What if I want to grep recursively for '-v':
shell.grep('-r', '-v', 'path/to/dir');In this case, '-v' will be treated as an option, which I don't want. So, using '--':
shell.grep('--', '-r', '-v', 'path/to/dir');But now '-r' is not treated as an option, which I also don't want.
There was a problem hiding this comment.
I guess, in the case grep specifically, this is how you do the above:
$ grep -r -- -v path/to/dirSo, -- is telling grep to take the next argument literally, as opposed to taking all arguments literally.
There was a problem hiding this comment.
No, see the added docs. The correct way to do what you describe is grep('-r', '-v', 'path/to/dir');
There was a problem hiding this comment.
Of course. I forgot that all arguments must be passed at once.
| if (opt === '--') { | ||
| // This means there are no options. | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
Given my realization above, what we need to do here is just ignore the next argument if '--' is present. This could be a bit tricky to implement though, looking at the way parseOptions works.
There was a problem hiding this comment.
No, there's at-most 1 option string in shelljs. That means that cp('-rf', 'foo', 'dir') is different than cp('-r', '-f', 'foo', 'dir'). The former copies one dir recursively and forcibly, the latter recursively copies 2 dirs named -f and foo into dir.
|
LGTM. |
|
Maybe there's something better we can do here. This makes sense for the no-options case: shell.grep('--', '-v', 'path/to/file.txt'); // just like unix, woohoo!But I agree this is a little confusing for the other case: shell.grep('-r', '-v', 'path/to/dir/'); // it looks like we're passing 2 flags, but '-v' is our regex string
// even worse, this is invalid shelljs code:
shell.grep('-r', '--', '-v', 'path/to/dir/'); // even though it looks like valid (but verbose) unixDo you think this API is sufficient? Is there a way to improve the readability of example 2? Should we allow example 3? |
|
I would say that because all of the flags must be passed in one string, that example 1 and example 2 are fine. But you are right, it does get confusing when example 3 would be valid in the shell but is not valid here. Maybe at some point in the future, we could allow flags to be passed in separately. But for now, I think this implementation is fine. If you're wondering, this is how I might implement allowing parameters that look like options, without using another argument (if we allowed passing in multiple options): // represents a literal value that might look like an option, like "-v"
function Literal(value) {
this.value = value;
}
// convenience method
function literal(value) {
return new Literal(value);
}
// some future implementation of parseOptions that handles
// multiple option arguments
function parseOptions(opts, map, errorOptions) {
// ...
opts.forEach(function (opt) {
// ignore the opt if we want to treat it literally
if (opt instanceof Literal) return;
});
// ...
}
// usage
shell.grep('-r', shell.literal('-v'), 'path/to/dir'); |
|
Probably fine as-is. I like the |
This adds the special option string
--, which means "no options". This can bepassed if the first parameter looks like an option (starts with a
-followedby 1+ letters).
Fixes #778