feat(glob): glob support for (almost) all commands#352
Conversation
src/common.js
Outdated
| function wrap(cmd, fn, options) { | ||
| // -1 means "don't glob" | ||
| // 0 means "glob the options" (probably don't want this) | ||
| var indexOfExpandableArg = { |
There was a problem hiding this comment.
This should be passed in as an option to common.wrap.
There was a problem hiding this comment.
Yeah, I was thinking about that approach as well. It's probably cleaner, so I'll modify it to do that instead. I'll get rid of the -1 stuff too.
|
One nitpick, otherwise is good! |
4d0e0e9 to
02324f6
Compare
|
@ariporad This should have your comments resolved. What do you think about globbing for Also, I'll probably add something about "globbing is supported for all commands" |
|
@nfischer: Yes for |
02324f6 to
3a7eb3f
Compare
|
@ariporad |
|
LGTM! |
feat(glob): glob support for (almost) all commands
Fixes #343. This performs glob expansion on commands, where it makes sense. This doesn't glob for:
pwd()(it doesn't take any arguments)test()(globbing here is very unreliable in bash)exec()(this actually does glob-expand, but that's done in the shell itself (/bin/sh), not by this functionset()(globbing doesn't make sense)tempdir()(it doesn't take any arguments)Some other commands we might want to not permit to glob (because it isn't useful, and I suspect globbing slightly hampers performance) would be:
which()mkdir()I had to modify some test cases that didn't actually have correct behavior (the
ls('path/*')cases were often wrong). I also added a lot of new test cases for globs on commands that didn't previously have them.I think this should be merged before #275, because it will help make sure that
glob.syncdoesn't break behavior.