-
Notifications
You must be signed in to change notification settings - Fork 70
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor #10
Refactor #10
Conversation
I'm about to remove a lot of the API, just committing first
optimizations
also passes all tests from minimatch and brace-expansion (and even passes some tests that bash fails on still)
using BASH environment variable in tests if available
I meant to comment earlier and got caught up with messing around with the tests... I like the changes a lot. I like how the things like parsers and compilers are split into smaller pieces that makes it easier to reason about and make changes when necessary. |
updating travis-ci configuration to install the latest bash based on the specific environment (osx or linux) adding osx to travis-ci matrix
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.
LGTM 👍
"never" | ||
] | ||
"wrap-iife": [2, "any"], | ||
"yoda": [2, "never"] |
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.
heh heh
to ensure they pass on windows
also: - expose parse/compile methods - add unit tests, including tests from 1.8.5
@jonschlinkert I just read the README and this looks so awesome! |
thanks! that's great, I really appreciate it! I'm glad it's working out, I spent a ton of time on this refactor trying to get it right. @doowb helped a lot too. It was fun, we actually wrote out the compiler steps character by character for a bunch of brace patterns before we even wrote a single line of code in the actual compiler. Not something I get to do often lol I'll try to write this quickly since my connection keeps going out (we've had thunderstorms for a couple of hours). the performance section captures the essence of what I meant when I said "as well as a couple of other reasons". The first point is that there are many, many scenarios where brace patterns grow geometrically. If you define two brace patterns for example, they're multiplicative if not exponential. So even though those patterns in the readme are contrived, it's not uncommon for users to define brace patterns that result in similarly huge strings. The second point is that there is potential for exploiting this effect in minimatch and the brace-expansion to cause a DoS (similar to the ReDoS bug minimatch had recently https://medium.com/node-security/minimatch-redos-vulnerability-590da24e6d3c#.jew0b6mpc). Basically what I'm getting at is that, for the reasons mentioned on glob-parent alone there is enough benefit in doing the brace expansion first, before passing patterns to minimatch or node-glob. Then if we add the two reasons I just listed it seems like a no-brainer. IMHO, minimatch should remove brace expansion before it gets exploited, or someone should do a pr to the (lol quickly... well I tried) edit: I just removed a partial sentence about bash/mac that didn't render because the code example wasn't escaped correctly. it didn't add to point anyway |
K I'm going to bump this to 2.0 and publish so I can push up the micromatch refactor :) |
Complete overhaul, with parser and compiler, source map support, more accurate matching (passes all Bash 4.3 brace expansion tests, all minimatch braces tests, all brace-expansion tests, and even passes tests that bash fails... and it's fast https://github.com/jonschlinkert/braces/tree/refactor#latest-results.
Still working on readme/docs.
@doowb, @hemanth, @es128, @eush77 any feedback or review would be appreciated. I'll squash this beast when it's time to merge (@phated you are of course welcome to leave feedback as well)
closes #8
closes #9