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 files option #3

Merged
merged 7 commits into from
Jul 24, 2017
Merged

Add files option #3

merged 7 commits into from
Jul 24, 2017

Conversation

kevva
Copy link
Owner

@kevva kevva commented Jul 3, 2017

Probably room for some improvement here and there, but it's functioning at least :). This is so we can use this in sindresorhus/globby#33.

@sindresorhus @SamVerschueren @schnittstabil

@kevva kevva requested a review from sindresorhus July 3, 2017 18:07
index.js Outdated
const getGlob = (fp, ext) => path.join(fp, '**', ext || '');
const getExt = ext => Array.isArray(ext) ? `*.{${ext.join(',')}}` : `*.${ext}`;
const getPath = fp => fp[0] === '!' ? fp.slice(1) : fp;
const isDirectoryP = pify(isDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using pify here, just use https://github.com/sindresorhus/path-type instead

Copy link
Owner Author

Choose a reason for hiding this comment

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

Used is-directory because it doesn't throw when the path doesn't exist. Could handle it here, but I thought it was cleaner not to.

Copy link
Contributor

Choose a reason for hiding this comment

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

path-type doesn't throw either, it just returns a boolean.

Copy link
Owner Author

@kevva kevva Jul 8, 2017

Choose a reason for hiding this comment

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

Yes, it does. You aren't handling ENOENT anywhere. Tests are failing when using it:

convert directories to glob - async

  Rejected promise returned by test. Reason:

  Error {
    code: 'ENOENT',
    errno: -2,
    path: 'foo/**',
    syscall: 'stat',
    message: 'ENOENT: no such file or directory, stat \'foo/**\'',
  }

If can do a PR if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. That is definitely not the intention. PR would be lovely :)

index.js Outdated
module.exports = (input, opts) => {
return Promise.all(arrify(input).map(x => isDirectoryP(getPath(x))
.then(isDir => isDir ? getGlob(x, opts) : x)))
.then(globs => globs.reduce((a, b) => a.concat(b), []));
Copy link
Contributor

Choose a reason for hiding this comment

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

[].concat.apply([], globs); instead?

@sindresorhus
Copy link
Contributor

How does the files option handle filenames with a dot? Like webpack.config where the full name is webpack.config.js.

@kevva
Copy link
Owner Author

kevva commented Jul 9, 2017

How does the files option handle filenames with a dot? Like webpack.config where the full name is webpack.config.js.

It doesn't atm. Any ideas?

@sindresorhus
Copy link
Contributor

We have to decide whether the extension is after the first or last dot.

@kevva
Copy link
Owner Author

kevva commented Jul 9, 2017

The last dot might be a better default since it'll cover most cases except .tar.gz etc. Maybe it should be configurable?

@sindresorhus
Copy link
Contributor

I would go with the last dot too. I don't think it should be configurable.

@kevva
Copy link
Owner Author

kevva commented Jul 21, 2017

Alright. What's your thoughts on sindresorhus/globby#46 (comment) to increase performance?

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
Copy link
Contributor

I don't think it's worth it. You don't usually have a lot of glob patterns, and few fs.stat checks are pretty cheap relatively to everything else involved in globbing. Using a regex will have ambiguous cases. The only way to be totally sure is to actually check the disk.

@kevva
Copy link
Owner Author

kevva commented Jul 24, 2017

Alright, then I think this is good to 🚢 ?

@sindresorhus sindresorhus merged commit 701f368 into master Jul 24, 2017
@sindresorhus sindresorhus deleted the files branch July 24, 2017 11:12
@sindresorhus
Copy link
Contributor

👍

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