Doesn't shell.js have its own glob expansion function?

Oh yeah, I'll fix that.

Fixed.

@ariporad Could you ping this with a push to trigger the CI?

@nfischer: Done.

Thank you!

@nfischer @ariporad as you can probably tell from package.json, I have generally biased to avoid dependencies. I have no problem with leveraging dependencies – esp for larger projects so you can get to market more quickly etc – but the downside of dependencies is that you lose a lot of control, in particular nuanced features and updates, and more often than not packages are abandoned/unmaintained (cough cough :)), so you end up blocking your own work until someone lands a feature upstream, etc.

luckily in our case our package is pretty small and the problems low level enough that the Node core lib is mostly sufficient. when I do encounter cases where reinventing the wheel takes more than a dozen lines of code or so, I simply copy and paste the relevant snippet :) like this: https://github.com/shelljs/shelljs/blob/master/src/cp.js#L44-L51

(it's obviously a tradeoff as in this case we also don't benefit from a well maintained project upstream, but again, those are all tradeoffs)

all this being said..... glob does seem like a good, well maintained library, and I think our code is reinventing the wheel a bit too much (and not that well), so let's go ahead and see if this is a good solution going forward.

my main concern at this point is breaking existing users, so let's bump at least the minor when we release, and make sure glob is mostly shell-compliant. (I'm not too familiar with ** behavior on *nix, so we'll need to check that).

thanks!

@arturadib: Three things:

1. Personally, I like lots of small modules, so I apologize in advance for when I forget on my PRs (Just remind me and I'll be more than happy to fix them. I'm not a fan of flame wars).
2. glob is written by @isaacs, the creator and CEO of npm (I'm not mentioning him to reduce his spam). No need to worry about it being abandoned.
3. According to semver, it should either be a patch bump if it doesn't break anything, or else a major bump (however, we're <1.0.0, which means that a instead of a major bump we would do a minor bump).

(I'm not mentioning him to reduce his spam)

Unfortunately, your clever and thoughtful plan failed. You need to put the \ between the @ and the name, or else GitHub will still send the notification. (Not to worry, I get basically an infinite amount of email, so one more or less makes no substantive difference ;)

The goal of the glob module is to match Bash 4.3 pattern matching as closely as possible, so it's probably a great fit for shelljs's needs. Since this will certainly change some of your existing edge cases, it seems like it probably should be a major version bump for shelljs, just to be on the safe side.

all this being said..... glob does seem like a good, well maintained library, and I think our code isreinventing the wheel a bit too much (and not that well), so let's go ahead and see if this is a good solution going forward.

I agree. My vote is in favor of switching to glob. I think if reinventing the wheel is simple enough, it may be best to do it ourselves. In this case however, we're not very closely matching Bash globbing, so the switch is probably smart.

It seems like we all agree we should give glob a shot. My main concern with this PR is that I'd like to move all globbing into common.js. Globbing is normally handled by the shell, not the utilities, in *nix, so handling all globbing in that file will simplify our code and almost certainly get closer to *nix behavior.

@nfischer: the more I think about it, the more I think that I want to submit a PR moving all the command boilerplate into common.js.

I'm unfortunately extremely busy this week, but over the weekend I should have some time to do that.

@nfischer: Ok, moved all the globbing into common, and fixed listing files with no dots.

Also rebased onto master.

@ariporad looks like there's some conflicts now (probably due to the -l option for ls). Once those are resolved, I can merge

@nfischer: Hmm... I actually think that there's no way to salvage -l. It's really tightly coupled to the existing implementation. I think we'll have to re-implement it.

@ariporad could you push this into a branch on the main repo? I'll take a look at refactoring -l support. I'd prefer to not lose any features if it can be avoided.

@nfischer: I did, but it won't work to update this PR.

@ariporad we would have to push to a new branch and open that as a PR to replace this one I think.

@nfischer: Yeah, or I can give you push access to my fork. Up to you. (I'd rather not loose all the discussion).

Yeah, or I can give you push access to my fork. Up to you. (I'd rather not loose all the discussion).

^ even better idea, actually

@nfischer: Ok, you've got push access.

I'll take a look

@nfischer: Ok, you've got push access.

So it turns out that this PR actually is kind of far from working. It passes CI because run-tests doesn't actually run any tests (ls() is broken). I have most of the tests passing, but I'm still running into some nasty edge cases. I think it may be more worth-while to do some refactoring first, and then make a simpler change that swaps common.expand()'s implementation for one that uses glob.sync().

@nfischer: that's what this does. (Take a look at the changes tab). What do you mean it doesn't work?

@ariporad this completely modifies the behavior of ls() as well. Click on any run of the CI and notice how it says "all tests passed"... without listing any of the tests. Now click on any other PR and see that run-tests lists every test file (i.e. cat.js, cp.js, etc) before the words "all tests passed"

@nfischer: Hmm... Let me look in to fixing that.

@ariporad I don't think you can wipe out all the ls() code. I think we would be much better off swapping the implementation of expand() with one using glob. I can add a call to expand() inside common.wrap(), which I where I think we should handle most expansion (it lifts the burden off utilities like rm(), which shouldn't need to worry about expand(), and vastly improves bash compatibility.

+1 on doing globbing (I'll take care of it).
It looks like this problem can be simplified to five cases we need to be able to handle:

1. foo (where ./foo is a directory, should be equivalent to foo/* [or foo/**/* if -r]).
2. foo/*.bar
3. foo/**/bar (where ./foo/<whatever>/bar is a directory, equivalent to foo/**/bar/*)
4. foo/**/bar (where ./foo/<whatever>/bar is a file without an extension, equivalent to foo/**/bar)
5. foo/**/bar/*.baz

Glob can handle 2, 4, 5, so we need to handle 1 and 3. I think the best way to do this is going to be to (pseudocode):

var output = [];
var results = glob(input);

for (var i = 0, len = results.length; i < len; ++i) {
    if (fs.statSync(results[i]).isDirectory()) {
        output.push(glob(results[i] + (options.recursive ? '/**/*' : '*')));    
    } else {
        output.push(results[i]);
    }
}

I think we should not handle cases 1 and 3 in common.expand(). The glob package works the way Bash would expand things at the command line. I think common.expand() should expand things just as the commandline would, and it should do that for every utility. Appending * to directory names is a pretty sloppy hack, and it's bound to cause us to deviate from Unix behavior sooner or later.

I'll write up a PR that puts a call to expand() inside common.wrap, and I'll take out extra expand() calls in the other utilities where possible. That way you can focus on swapping out the internals of common.expand() with glob.sync, and making sure we have compatibility.

Edit: I accidentally referred to common.extend() instead of common.expand() (and they're fairly different).

@nfischer: Agreed. But don't submit the PR (I've already got 75% of it).

Whoops, I didn't mean to close this. Nor post that last comment.

I'm going to demote this to v0.7.0, because we've got some kinks to work out.

Just for future reference, this was closed in favor of #359