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

Switch to micromatch: fixes yarnpkg/yarn#3336 #3339

Merged
merged 5 commits into from
May 12, 2017
Merged

Switch to micromatch: fixes yarnpkg/yarn#3336 #3339

merged 5 commits into from
May 12, 2017

Conversation

jrop
Copy link
Contributor

@jrop jrop commented May 6, 2017

Summary

The micromatch library provides an inconsistent implementation of .makeRe(...) with the rest of it's interface. It is known that makeRe does not produce Regular Expressions that can be trusted:

This PR switches from minimatch => micromatch.

Motivation: #3336 😄

Test plan

TODO:

  • Update tests and make them pass (before merge)

Tested so far:

$ alias my_yarn='node /github/yarn/lib/cli/index.js'
$ cd /my/other/project
$ my_yarn pack
$ tar -tf my-package-v1.0.0.tgz
#
# Output is as expected
#

@jrop
Copy link
Contributor Author

jrop commented May 8, 2017

Unpredictable tests on Travis fail with "timeout" errors:

Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
      
      at ontimeout (timers.js:386:14)
      at tryOnTimeout (timers.js:250:5)
      at Timer.listOnTimeout (timers.js:214:5)

This failure does not seem to be related to this PR, am I correct?

Jonathan Apodaca and others added 2 commits May 12, 2017 09:11
@bestander bestander merged commit d223116 into yarnpkg:master May 12, 2017
@bestander
Copy link
Member

Nice, thanks for a thorough explanation, tests and fix

@jrop
Copy link
Contributor Author

jrop commented May 12, 2017

@bestander 😰 I just realized that the way I added the extra test causes it to fail on windows in AppVeyor, because I hard coded the path separator lib/a.js for example. On windows the test fails because the path will be lib\a.js. I pushed one more commit that changes the test to use path.join('lib', 'a.js'). Does it need another PR?

@bestander
Copy link
Member

Yeah, could you send a new PR?

@bestander
Copy link
Member

@jrop when to expect the PR?
I'd like to cut 0.25 release

@jrop
Copy link
Contributor Author

jrop commented May 12, 2017

@bestander Just submitted. Sorry about that ☹️

@bestander
Copy link
Member

no worries :)

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