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

refactor: switch minimatch to micromatch #3538

Merged
merged 1 commit into from Jul 9, 2019

Conversation

@curbengh
Copy link
Contributor

curbengh commented Apr 25, 2019

What does it do?

micromatch is a replacement of minimatch. It supports multiple patterns in an array, without string-to-array conversion, along with other globbing features. It's also faster and updated much more frequently (micromatch vs minimatch). It's already utilized in hexo, indirectly through chokidar dep. npm ls --depth 5 shows 4 instances of it.

Using micromatch@4 is rather drastic since it depends on Node >=8, considering other deps currently use micromatch@3. But it's not a major issue, as Node 6 will be eventually dropped (#3508).

Personally, I would prefer to enable the matchBase option. With this option, I can just use 'ignore-this.txt' to exclude that file (assuming the filename is unique. I'm aware of the underscore ignore), instead of '**/ignore-this.txt'. When globbing is used, if I want to exclude all .txt, I only need '*.txt', instead of '**/*.txt'. In the case of #3306, it can be just 'abc*'. The doc needs to be updated though if this behavior is introduced.

How to test

git clone -b micromatch https://github.com/weyusi/hexo.git
cd hexo
npm install
npm test

github page: https://github.com/weyusi/hexo-testing

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.
    • will not pass in Node 6 environment.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 25, 2019

Coverage Status

Coverage decreased (-0.002%) to 97.146% when pulling 8d83bdf on weyusi:micromatch into c2e4672 on hexojs:master.

@YoshinoriN

This comment has been minimized.

Copy link
Member

YoshinoriN commented May 3, 2019

@weyusi
Thank you very much :)
As you know we haven't decided to drop Node.js v6... Please wait for a while.

@tcrowe

This comment has been minimized.

Copy link
Contributor

tcrowe commented Jun 20, 2019

I like the idea and your arguments are valid, @weyusi. Thank you for the idea and PR.

@curbengh curbengh changed the title refactor: switch minimatch to micromatch WIP refactor: switch minimatch to micromatch Jun 29, 2019
@YoshinoriN YoshinoriN added this to the v4.0.0 milestone Jul 1, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jul 1, 2019

is matchBase option okay?

Edit: I just tested matchBase option. While it's useful to ignore a filename in any folder, it doesn't work if a folder needs to be specified (e.g. **/foo/bar.txt or **/*/foo/bar.txt).

@curbengh curbengh force-pushed the curbengh:micromatch branch from ffc2940 to 8d83bdf Jul 9, 2019
curbengh pushed a commit to curbengh/hexo-generator-sitemap that referenced this pull request Jul 9, 2019
@curbengh curbengh changed the title WIP refactor: switch minimatch to micromatch refactor: switch minimatch to micromatch Jul 9, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jul 9, 2019

I've rebased. Ready to be reviewed.

@tcrowe
tcrowe approved these changes Jul 9, 2019
Copy link
Contributor

tcrowe left a comment

I tested it and it worked 👍🏻

Thank you @weyusi

@tcrowe tcrowe merged commit f172774 into hexojs:master Jul 9, 2019
3 of 4 checks passed
3 of 4 checks passed
coverage/coveralls Coverage decreased (-0.002%) to 97.146%
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@curbengh curbengh deleted the curbengh:micromatch branch Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.