- Notifications
You must be signed in to change notification settings - Fork744
Provide an API to pass parameters which resemble options#792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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
freitagbr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmmm... What if I want togrep 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess, in the casegrep specifically, this is how you do the above:
$ grep -r -- -v path/to/dir
So,-- is tellinggrep to take the next argument literally, as opposed to taking all arguments literally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, see the added docs. The correct way to do what you describe isgrep('-r', '-v', 'path/to/dir');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 wayparseOptions works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, there's at-most 1 option string in shelljs. That means thatcp('-rf', 'foo', 'dir') is different thancp('-r', '-f', 'foo', 'dir'). The former copies one dir recursively and forcibly, the latter recursively copies 2 dirs named-f andfoo intodir.
freitagbr commentedOct 20, 2017
LGTM. |
nfischer commentedOct 20, 2017
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) unix Do you think this API is sufficient? Is there a way to improve the readability of example 2? Should we allow example 3? |
freitagbr commentedOct 20, 2017
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"functionLiteral(value){this.value=value;}// convenience methodfunctionliteral(value){returnnewLiteral(value);}// some future implementation of parseOptions that handles// multiple option argumentsfunctionparseOptions(opts,map,errorOptions){// ...opts.forEach(function(opt){// ignore the opt if we want to treat it literallyif(optinstanceofLiteral)return;});// ...}// usageshell.grep('-r',shell.literal('-v'),'path/to/dir'); |
nfischer commentedOct 27, 2017
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