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

Option to only return files #33

Closed
sindresorhus opened this issue Aug 14, 2016 · 16 comments · Fixed by #46
Closed

Option to only return files #33

sindresorhus opened this issue Aug 14, 2016 · 16 comments · Fixed by #46

Comments

@sindresorhus
Copy link
Owner

Currently, globby or rather glob returns both directories and files. You can turn off returning directories using the nodir: true glob option, but that's not what you want. You'd rather want to expand directories into files. That's what this option should do. So globby.sync('foo') where foo is a directory would be expanded into all the files in that directory that doesn't match any of the ignore patterns. Same with globby.sync('foo/**'). There also needs to be a way to specify an array of extensions. In many cases you wouldn't want the expansion to happen for all files, but, for example, just .js files. It should override the nodir option if specified by the user.

This would be useful and simplify code in AVA, XO, cpy, imagemin-cli, and other tools expecting files. For example: https://github.com/avajs/ava-files/blob/master/index.js#L212-L271

@UltCombo @schnittstabil @jamestalmage @SamVerschueren @novemberborn Thoughts?

@UltCombo
Copy link
Contributor

UltCombo commented Aug 14, 2016

I'm neutral on this one.

You have basically described Chokidar's matching behavior—when it matches a directory, all descendant files and directories are included. In Chokidar, you can set a depth option to specify how many levels of subdirectories will be traversed (depth: 0 disables traversing subdirectories).

Though, I'm not exactly sure how this approach is better than the current ones.

To match all .js files descendant of a given directory, I'd normally prefer using a standard glob pattern (by appending /**/*.js to the end of the directory path) instead of a globby-specific API.

I may have not fully understood your suggestion. Can you post a sketch of how you think this API would look like and what problem is it solving (or is it actually just hiding the path + /**/*.{${extensions.join(',')}}`` complexity)?

@sindresorhus
Copy link
Owner Author

@UltCombo It's solving the problem that people specify directories in tools and expect it to work. Most people don't fully understand globs. I also want it to work that way. $ foo dir is a lot nicer and user-friendly than $ foo dir/**/*.

or is it actually just hiding the path + /**/*.{${extensions.join(',')}} complexity

Yes

To be clear, this will be an opt-in option. Name onlyFiles (?)

@UltCombo
Copy link
Contributor

@sindresorhus I see. I agree about the CLI friendliness.

I'm just not sure if this should be handled in globby, in the caller or in a different package (my glob-manipulate package comes to mind).

Regarding the name: onlyFiles could be misleading in the same manner that you find node-glob's nodir option misleading. Maybe expandDirectoriesIntoFiles: Boolean | String[].

@sindresorhus
Copy link
Owner Author

@UltCombo I think it should be handled here. I made globby with the intention of making globbing more human friendly. This is fully in the scope of that. Lots of tools now have duplicated logic to handle this case already. Better have it done properly in one place.

Lets go with expandDirectories as a name then.

@UltCombo
Copy link
Contributor

Yeah, I fully agree on deduping this logic and doing it properly in a single place.

To be clear, when I asked whether this should be handled in globby or in the caller, I'm also considering the possibility of globby or the caller including a dependency specifically for this glob manipulation task.

globby is not the only node-glob abstraction, so implementing such glob manipulation tasks here mean that this work may be duplicated in other node-glob abstractions. I've created glob-manipulate to avoid such duplication.

@novemberborn
Copy link

Sure, this sounds useful.

@schnittstabil
Copy link
Contributor

So globby.sync('foo') where foo is a directory would be expanded into all the files in that directory…

If I have understood correctly, expandDirectories: true will imply nodir: true for foo, I think that isn't appropriate in most use cases:

  • At least for cpy, we need all files and all (empty) directories. Similar may hold for AVA and XO (some day?), e.g. to complain about empty test/source directories.
  • Moreover, it is not a great advantage for tools like imagemin-cli, which only process files, they still can use nodir: true in conjunction with expandDirectories.

I like the extension idea, but using globs seems more natural to me:

expandDirectories: ['test_*.js', '*.jsx']

@sindresorhus
Copy link
Owner Author

If I have understood correctly, expandDirectories: true will imply nodir: true for foo, I think that isn't appropriate in most use cases:

Most tools I've created process files only. cpy is really the exception. I'd be fine not overwriting nodir, but I think it should be true by default unless explicitly set.

@sindresorhus
Copy link
Owner Author

I like the extension idea, but using globs seems more natural to me:

@schnittstabil This would mean globby.sync(['foo'], {expandDirectories: ['test_*.js', '*.jsx']}) would result in the following globs: foo/**/test_*.js and foo/**/*.jsx, right? If so, like it. I would still like a way to just specify just extensions though, as that's the most common need.

Maybe also support the following format:

expandDirectories: {
	files: ['test_*'],
	extensions: ['js', 'jsx']
}

What do you think about setting nodir to true by default? It will be clearly documented, but I think that's the most common use-case, and I've been victim of assuming it's already the default multiple times.

@stevenvachon
Copy link

stevenvachon commented Jun 22, 2017

Shouldn't we instead add this option to glob? That way all this lib has to do is perform a union on the returned arrays, or memoize if we stream with glob.Glob.

Aside from that, a path to a dir means all files within, with no customization:

cp -r /path/to/dir/ /dest/

is the same as:

cp -r /path/to/dir/**/* /dest/

@sindresorhus
Copy link
Owner Author

Shouldn't we instead add this option to glob?

Preferably yes, but glob moves way too slowly, if at all, so I'm gonna focus my energy here.

@stevenvachon
Copy link

What part of glob moves too slow? glob.Glob is faster than glob().

@sindresorhus
Copy link
Owner Author

The project, not the code.

@stevenvachon
Copy link

Oh, ok. Do you intend to rewrite glob and not depend on it?

@sindresorhus
Copy link
Owner Author

No, this can be implemented upon glob, like the other things in this module.

@stevenvachon
Copy link

isaacs/node-glob#344

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants