consider a PR to replace brace expansion?

[jonschlinkert]

If you're open to it, I'd like to do a PR to replace the current brace expansion logic with braces.

The primary reason I'd like to make this PR is b/c we use minimatch and glob extensively, so - for selfish reasons - I'm attempting to make it easier for me (and hopefully others) to contribute by modularizing/streamlining some of the code.

I already converted everything locally and all related tests pass. Braces has better test coverage if you want to see those tests as well. I also created some basic benchmarks:

I'd be happy to do a PR for your review first if you're open to this.

[isaacs]

Faster is great, and I'm all about modularizing pieces.

Of course, it'd have to not break any of minimatch or glob's existing set of tests. I can see that this might raise an issue, since braces throws on unbalanced braces, unlike minimatch or glob:

$ node -e 'require("braces")("{a,b{,c}")'

/Users/isaacs/dev/js/x/node_modules/braces/index.js:67
        throw new Error(msg + str);
              ^
Error: [brace expansion] imbalanced brace in: {a,b{,c}
    at braces (/Users/isaacs/dev/js/x/node_modules/braces/index.js:67:15)
    at module.exports (/Users/isaacs/dev/js/x/node_modules/braces/index.js:21:10)
    at [eval]:1:18
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:456:26)
    at evalScript (node.js:532:25)
    at startup (node.js:80:7)
    at node.js:901:3

$ echo {a,b{,c}
{a,b {a,bc

[isaacs]

Turns out that minimatch isn't doing quite the right thing here either.

$ node -p 'M=require("minimatch").Minimatch; new M("{a,b{,c}").set'
[ [ '{a,b{,c}' ] ] // <-- expect [ [ '{a,b', '{a,bc' ] ] to match bash 4.3

but at least it doesn't throw, which is much more aggressive.

[isaacs]

Another inconsistency:

$ echo a{b}c
a{b}c

$ node -p 'require("braces")("a{b}c")'
[ 'abc' ]

I'd recommend running through all of the (admirably extensive!) braces tests and comparing them to the behavior of Bash 4.3. If it can be modified to match that standard, then that'd make it a much easier call to migrate minimatch to use it. Of course that'd be a breaking change, but that just means you'll have to bump the major version number. Otherwise, I think it'll be a tough sell, since it'd break node-glob, where the explicit contract is "behaves the same as Bash 4.3".

[jonschlinkert]

I can see that this might raise an issue, since braces throws on unbalanced braces

Honestly I wasn't sure what to do on those. Instead, I can have braces not throw, or make that an option in debugging.

Another inconsistency:

Forgot to to mention this one, a{b}c. It seemed that abc would be the expected result. and fwiw, I had a situation where a{b}c would be desirable (I was creating brace patterns from arrays, [].join(','), that might only have one element), but couldn't think of a reason that a{b}c would be problematic. But I'll do as you suggest and look at Bash 4.3 and use that as canon.

thanks, I'll report back after I look over bash and update braces to not throw.

[jonschlinkert]

Regarding the imbalanced braces, since neither lib is doing the idiomatic thing, what do you think we ought to be doing there? I can just make it consistent(ly broken lol) with minimatch, and/or we can add an option to throw in debugging mode or something...

[isaacs]

I consider bash to be authoritative here, so this is a bug in minimatch. If we can fix that bug by using braces then that'd be an even stronger argument in favor of your proposal :)

[jonschlinkert]

👍 thx.

Just to keep you updated on my approach in case you have advice, I created a script to generated test fixtures/expected from bash 4.3: https://gist.github.com/jonschlinkert/59e5d8852e2ad7faebaa

This is rough, so it needs to be manually cleaned up/fixed next.

However, at a glance I don't think either lib would pass on a some of these. namely, https://gist.github.com/jonschlinkert/59e5d8852e2ad7faebaa#file-fixtures-js-L105-L137. But IMHO, the absence of these does not make minimatch "broken", it's a matter of implementing these as features when requested with valid use cases.

[isaacs]

I think that including any sort of command substitution is probably inappropriate for these libs. So, for example, XXXX\{echo a b c | tr ' ' ','\} should not expand to XXXX\{a,b,c\}, but instead treat ``` as just another quote character, the same as ' or `"`.

Some others that I think braces and minimatch both do improperly: {01..100} {0001..5}. Note that the padding should be enough to out to the maximum of either the start or end string length.

Would you be open to PRs to braces/expand-range to make them behave the same as bash? That'd probably be the best first step to get this into minimatch, and I think it would improve glob (and thus gulp, grunt, npm, etc)

[jonschlinkert]

Okay, I've made some updates and implemented n to the shload unit tests. See the minimatch branch of braces: https://github.com/jonschlinkert/braces/tree/minimatch, and tests

• no longer throws
• implemented 70+ unit tests from Bash 4.3. The only tests excluded were those with $(zecho foo {1,2} bar) or similar variables/commands that seem IMHO to be irrelevant
  • braces: passes: 62, fails 12 (lol math doesn't match, I need to check which is missing)
  • minimatch - passes: 43, fails: 32

There are 5 tests that minimatch passes and braces fails, two of which are due to sorting order and the rest of which don't seem like they should be solved. At one point this morning I had 70 of the tests passing at the expense of a 50-60% reduction in speed. IMO, it's not worth it. Any additional coverage would (again, IMHO) only be justified with actual, valid use cases. But I'll leave you to be the judge of that.

Updated benchmarks (sans one lib that doesn't pass most of the tests anyway)

[jonschlinkert]

Note that the padding should be enough to out to the maximum of either the start or end string length.

I didn't see unit tests in bash for actual padding inside the braces, only "faux" padding, e.g 0{1..5}. So I actually removed support for it. I can add it back if it's important. (edit: or desirable...)

Would you be open to PRs to braces/expand-range to make them behave the same as bash?

Absolutely! You might want to check what I did with braces, and also I split out the actual range expansion to https://github.com/jonschlinkert/fill-range, since this kind of range expansion can be useful for more than splitting strings with ...

for example, XXXX{echo a b c | tr ' ' ','} should not expand to XXXX{a,b,c}, but instead treat `

No prob, I'll follow your lead regarding expectations here

[isaacs]

So, I looked around, and found that Julian Gruber's brace-expansion module is actually a closer fit to bash brace expansion. I spent a bit of time while traveling to put together a few pull requests, and I think it's where it needs to be to make minimatch and glob better at this.

It's probably not as fast as braces, but I suspect that (a) that won't affect glob's or minimatch's performance too much, and (b) there's probably plenty of low-hanging fruit that can be profiled and fixed up to make it much faster. Maybe it'd be worthwhile to port your benchmark from braces, and we can figure out where the bottlenecks are?

In the meantime, correct behavior is more important than fast brace expansion, so I think I'm gonna go with that implementation instead for now.

[jonschlinkert]

and found that Julian Gruber's brace-expansion module is actually a closer fit to bash brace expansion... It's probably not as fast as braces...correct behavior is more important than fast brace expansion

Being faster wasn't "the goal". I created braces to be "correct", since brace-expansion doesn't pass 75% of the unit tests, nor did minimatch or any other lib I tested. I just optimized afterwards and removed support for features that will never be used by node.js users.

[jonschlinkert]

since brace-expansion doesn't pass 75% of the unit tests,

actually this might have been before I changed all the tests and the lib's per your request in #52 (comment). For all I know it passes 100% of those tests. I'll have to check when I get home later tonight.

That said, braces passes 92 of 98 of the actual Bash 4.3 spec's tests. If I'm correct that braces passes more of the tests, then - following your own words about bash compliance - I assume you'll still use braces?

[isaacs]

As of 1.0.0, brace-expansion passes all of the test cases that I could throw at it. It doesn't support quotes, but it's unclear how that should be handled in a JavaScript lib.

Bash's built-in test suite is not a "spec" per se. It's actually quite a lacking test suite. But if there are any in there that brace-expansion 1.x doesn't pass, let's get that fixed.

[jonschlinkert]

all of the test cases that I could throw at it

awesome, can you link to those for the rest of us? It fails a number of the actual tests I published.

Bash's built-in test suite is not a "spec" per se. It's actually quite a lacking test suite.

Sorry to be blunt, but no sh**. and it's not an appropriate spec to follow node.js. But since you said you wanted this to be 100% bash compliant, I made that the goal of the pr because I use minimatch a lot.

But if there are any in there that brace-expansion 1.x doesn't pass, let's get that fixed.

Why? Why would I - or you - spend time fixing another library that is not as good, comprehensive, or as fast as one I already have? Go for it though, if that's how you want to spend your time.

[jonschlinkert]

wait, and now I see that the reason it's passing tests now is because you contributed to it over the last few days!? wft. Honestly what a douchebag thing to do. After months of stagnation in that lib and this one, I offer to fix this problem, you would rather fix a buddy's lib than to encourage the spirit of contributing and collaboration? wow.

@isaacs you are supposed to be a leader and example in the node.js community. Thanks for reminding me why I should spend my time creating good quality libs from scratch than to rely on - or try to better - old, bloated libs like this that are only popular because they've been around since the early days of node.

[isaacs]

I understand you're upset. For what it's worth, I looked at the work required to make both braces and brace-expansion pass these tests, and it was just less refactoring this way, and already used a test framework that I was more familiar with.

It's not personal, and has nothing to do with any relationship I have with Julian, who I barely know. For all I knew, he could've just rejected my changes outright, and we'd have 3 brace expansion libs now. Coding is mostly a hobby for me these days, so I chose the path that was the least amount of effort.

I am sorry that I didn't communicate this ahead of time.

[jonschlinkert]

It's not personal, and has nothing to do with any relationship

This is nothing about anything personal. I'm not upset that you're not using my code. What actually started to get me irritated was this:

In the meantime, correct behavior is more important than fast brace expansion, so I think I'm gonna go with that implementation instead for now.

Correct behavior? Really? I have actual stats - which I've shown - proving that not only is braces faster, but it also has more "correct behavior" - and not just by a little margin. The reason this statement is problematic is that your words carry a lot of weight in the community. Many people will just believe your statement and accept it at face value, and I now have a library that has been labeled by "isaacs" as having "incorrect behavior". But your statement itself is the only thing that is incorrect.

Regarding your actual decision, yeah I'm kind of offended that you've actually been making an pretty solid effort to not use my code, despite my demonstrated willingness to close the gap with features and despite me showing you stats and unit tests that prove my code is better.

But hey, this is open source, right? No one is obligated to merge in prs...

So more to the point, what I care about the most is that I have to keep using this crap now. Because despite trying not to use my code, and despite trying to improve another library to do what braces already does, braces is still faster and more complete, and as a result, I and the rest of the minimatch user base (all of node.js?) is forced to keep using your hobby when we could have something better right now, or until someone produces a better lib.

Here are those stats I mentioned:

Unit test stats (for Bash 4.3 support)

braces:

• pass: 95
• fail: 8

brace-expansion v0.0.0 (before commits from @isaacs, Braces was already passing 95 tests at the time)

• pass: 72
• fail: 31

brace-expansion v1.0.0 (after commits from @isaacs)

• pass: 85
• fail: 18

minimatch

• pass: 55
• fail: 48

Clearly when you say "correct behavior", you mean something other than support for Bash 4.3. ... Meanwhile, in the time you spent attempting to improve the other lib to be as complete as braces, I could have easily added support for the 8 failing tests and whatever other features were missing.

Plus, I already mentioned somewhere that I had braces passing all but 2 but I removed some code because it seems silly to support features that can't or won't be used in node.js, "just to be complete" at the cost of a 20-30% speed reduction. To that end...

benchmarks

So, not only is braces ~600% faster for typical use cases, but it has more "correct behavior", and supports other features that none of the other libs support - like passing a custom function to modify each value as it's expanded.

Back to a comment you made earlier:

Maybe it'd be worthwhile to port your benchmark from braces, and we can figure out where the bottlenecks are?

You're free to use whatever you want, since my projects are all MIT licensed. But, why? Why try to use things from my project(s) to benefit an inferior, similar project when you could have just used my actual project instead? It's not about competition, I love open source. It's about making good use of our time, being good stewards of our projects, and trying to think about our users and contributors first.

[isaacs]

"Correct behavior" for the purposes of minimatch is "identical to Bash 4.3". It's not a statement of code quality or any sort of pejorative judgement. Bash is indeed an arbitrary (and arguably strange) standard to follow, but since minimatch is used in contexts where the command line is the standard, it's a sensible one.

I created this suite of test cases: https://github.com/isaacs/brace-expansion/blob/master/test/cases.txt This script creates the bash results: https://github.com/isaacs/brace-expansion/blob/master/test/generate.sh and this one compares the JS impl to them: https://github.com/isaacs/brace-expansion/blob/master/test/bash-comparison.js

I then took a look at the cases that were failing in both braces, brace-expansion, and the existing minimatch implementation. The failing cases in brace-expansion turned out to be more straightforward to address, even though there were more of them, mostly because the brace-matching strategy was already factored out, and most of the failures were in that area. I spent a bit of time this weekend making the required changes, julian decided to accept them, and here we are.

While you say it's not about competition, you are pretty insistent that braces is better. That makes me think that you are maybe feeling a bit competitive or rejected. Again, I apologize for not communicating clearly. I see that this gave you the impression that I had decided to use the module you wrote.

[jonschlinkert]

While you say it's not about competition, you are pretty insistent that braces is better. That makes me think that you are maybe feeling a bit competitive or rejected.

I'm not feeling rejected or competitive, I'm feeling bewildered that you're ignoring actual facts. Please stay on topic and stick to facts instead of trying to distract from the issue.

The failing cases in brace-expansion turned out to be more straightforward to address

Understandable. That lib had so many issues to address I'm sure it was easy to find a place to dive in and start fixing things. Meanwhile, braces still passes more of the tests. And braces is still faster.

[isaacs]

Ok, let's stick to the facts, then. The fact is that we have different priorities, and that's fine.

According to your benchmarks, braces is 400% faster. Ok, great. Not relevant to me. Doesn't make a difference to glob, which is what I care about.

You say that braces "passes more tests" now. But it doesn't pass the suite of test cases I linked to, and you closed an issue stating that you aren't interested in making it behave that way.

Braces factors out the range expansion. Brace-expansion factors out balanced matching. That made it easier for me to get it to pass the tests that I need it to pass. I also started working with braces to make it behave that way, but it would've been a much larger refactor. Since I was assuming I might end up inheriting the lib's maintenance, I chose the one that was structured in a way that was easier for me to work with.

What do you want to accomplish in this discussion? I really would like to figure out how to leave you feeling less bewildered.

From my point of view, that is "the issue" that I'd like to stick to.

The technical stuff is subjective and arbitrary, and not very interesting to talk about at this point. I understand that you disagree with my technical choice here, and that's fine. There's no need to keep debating it. I've given you my reasons, we can move on from that.