Skip to content
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

ISSUE-116: Broken RegExp for pattern with noext option #117

Merged
merged 1 commit into from
Feb 18, 2018
Merged

ISSUE-116: Broken RegExp for pattern with noext option #117

merged 1 commit into from
Feb 18, 2018

Conversation

mrmlnc
Copy link
Contributor

@mrmlnc mrmlnc commented Feb 16, 2018

Links

What is?

When we work with the text node – we shouldn't add a backward slash before it.

For example, the following pattern:

./test(dir)/foo.txt

Before fix:

/^(?:(?:(?:\.(?:\/|\\))(?=.))?test\(\dir\)\/foo\.txt)$/

After fix:

-/^(?:(?:(?:\.(?:\/|\\))(?=.))?test\(\dir\)\/foo\.txt)$/
+/^(?:(?:(?:\.(?:\/|\\))(?=.))?test\(dir\)\/foo\.txt)$/

Questions

  • Sorry but I don't understand where I should add the new test 😭

@jonschlinkert
Copy link
Member

Hi, thanks for the pr! There is an extglob edge case that will break with this change, I'm not sure why travis is passing, it's failing locally for me.

If we update that code to only escape parens and non-word characters everything will work as expected:

  compiler.set('paren', function(node) {
    var val = '';
    visit(node, function(tok) {
      if (tok.val) val += (/^\W/.test(tok.val) ? '\\' : '') + tok.val;
    });
    return this.emit(val, node);
  });

Sorry but I don't understand where I should add the new test

You added the test to the right place, thanks!

Do you want to update your PR? If not I can push it up.

@jonschlinkert
Copy link
Member

Note that globby does not use micromatch.

@mrmlnc
Copy link
Contributor Author

mrmlnc commented Feb 18, 2018

it's failing locally for me.

I have no any problems locally on the macOS with Node.js 9.5.0.

Do you want to update your PR?

Done.

Note that globby does not use micromatch.

Now the globby package uses fast-glob package that uses micromatch :P

@jonschlinkert
Copy link
Member

Now the globby package uses fast-glob package that uses micromatch :P

ah! I missed that! thanks!

Btw the failures are expected. I just optimized a couple regexps in nanomatch, I just need to update those unit tests.

@jonschlinkert jonschlinkert merged commit 3620e87 into micromatch:master Feb 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants