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

Add expandDirectories option and set nodir to true by default #46

Merged
merged 3 commits into from
Oct 22, 2017

Conversation

kevva
Copy link
Contributor

@kevva kevva commented Jul 4, 2017

Noticed a pretty large performance regression so might have to fix that first before merging. Would've liked to to implement the login in generateGlobTasks but we'd need to introduce generateGlobTasksSync in that case.

Fixes #33.

@kevva
Copy link
Contributor Author

kevva commented Jul 4, 2017

Ok, think I've nailed the performance regressions to the nodir option being true, and the isDirectory call here https://github.com/kevva/dir-glob/blob/0a3b078f088836d9584510d1c735875dac9dc98b/index.js#L45.

I'm wondering if we maybe should check if it's a glob using a regex before checking if it's a directory in the dir-glob module?

@sindresorhus sindresorhus mentioned this pull request Jul 5, 2017
package.json Outdated
@@ -53,6 +53,7 @@
],
"dependencies": {
"array-union": "^1.0.1",
"dir-glob": "kevva/dir-glob#files",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that branch hasn't been merged to master yet.

@sindresorhus
Copy link
Owner

I'm wondering if we maybe should check if it's a glob using a regex before checking if it's a directory in the dir-glob module?

It's hard to reliably do that. Let's skip that for now. I don't want to deal with the headache of false negatives.

@sindresorhus sindresorhus changed the title Add expandDirectories option Add expandDirectories option and set nodir to true by default Oct 22, 2017
@sindresorhus sindresorhus merged commit cf55e5c into master Oct 22, 2017
@sindresorhus sindresorhus deleted the dir-glob branch October 22, 2017 09:37
@sindresorhus
Copy link
Owner

Looks good! Glad we could finally include this in globby 😎

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.

Option to only return files
3 participants