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

Adding multiple directory functionality #12

Closed
wants to merge 6 commits into from

Conversation

DomLennonZA
Copy link

Specifying multiple directories with a file filter did not work. This adds that functionality.

@tonistiigi
Copy link
Owner

Hi

Please repost without the .idea folder and semicolon style changes.

This seems to be Windows specific(taken from the backslash). Normally your shell should handle wildcards automatically. I'm ok with supporting this but it has to work on non-windows as well and at the moment I think it only works if wildcard is followed by a dot. I think better solution would be to just include https://github.com/isaacs/node-glob as a dependency.

@DomLennonZA
Copy link
Author

Sure, will check out Glob. Why do you leave out the semicolons though?

@tonistiigi
Copy link
Owner

Why do you leave out the semicolons though?

Because you don't need them! Anyway, you can find plenty of discussions about whether to use them or not. I don't want to start another one. Main thing is that a repo should have a consistent style and theres no reason for conflicting all merges for something that adds no real value.

@DomLennonZA
Copy link
Author

Build passed. This has been tested on PC and Mac machines and is working as expected. Please review again.

@tonistiigi
Copy link
Owner

Issues I have with this:

  • This seems to be windows only(because it searches for a backslash).
  • It only accepts *.ext pattern.
  • It falsely matches files in the directory that contain extension in the filename(but not in the end).
  • Filenames that contain * are perfectly valid under Linux/OSX. This change invalidly turns them into wildcards(even if escaped).

I tested the wildcard inputs in Git bash under Windows and it worked fine. If you see this under Command Prompt then that is not really something I want to support(as stated in the readme).

@bassarisse
Copy link
Collaborator

Would it be better to have an option to enable recursive search for the files?

@tonistiigi
Copy link
Owner

I don't think its worth it. To me the problem clearly seems to be in the broken shell implementation and has nothing to do with audiosprite. It would just complicate the codebase and usage.

I think Git bash is a very good option for Windows users. In addition to to wildcards it also makes managing PATH env variable simpler, has better completion support etc.

@zlumer
Copy link
Collaborator

zlumer commented Feb 27, 2016

Fixed in #41 (v.0.6.0 on NPM)

@zlumer zlumer closed this Feb 27, 2016
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.

4 participants