Skip to content

adds support for filter function#1

Closed
jonschlinkert wants to merge 3 commits intogulpjs:masterfrom
jonschlinkert:master
Closed

adds support for filter function#1
jonschlinkert wants to merge 3 commits intogulpjs:masterfrom
jonschlinkert:master

Conversation

@jonschlinkert
Copy link
Contributor

to provide control over eliminating files like .DS_Store on mac or Thumbs.db on windows from causing an unexpected result. Also:

  • adds exists function, since directories that do not exist should return false.
  • updated metadata in package.json per latest npm recommendations and conventions
  • unit tests for filter function
  • docs for filter function

If this is too much for this lib, no worries. If it's okay, that would be great, I'd love to use this in verb and generate. thanks!

to provide control over eliminating files like `.DS_Store` on mac or `Thumbs.db` on windows from causing an unexpected result. Also:

- adds `exists` function, since directories that do not exist should return `false`.
- updated metadata in package.json per latest npm recommendations and conventions
- unit tests for filter function
- docs for filter function
@jonschlinkert
Copy link
Contributor Author

oh, one more thing... I used the try-open lib for the exists check, since fs.exists is deprecated

@tkellen
Copy link

tkellen commented Jan 26, 2016

hey @jonschlinkert, this looks nice! one question for ya though... why the preference for excluding .DS_Store automatically but not Thumbs.db?

@jonschlinkert
Copy link
Contributor Author

Just my best guess at correct behavior, mainly since .DS_Store is created by the OS automatically after a folder is created, but Thumbs.db will only be there after images have been in the folder at some point.

@tkellen
Copy link

tkellen commented Jan 28, 2016

Gotcha. Would you be willing to remove the .DS_Store addition for OSX? It feels like an overreach. I'm 👍 to including a sample for that in the docs though!

@tkellen
Copy link

tkellen commented Jan 28, 2016

PS: I've just added you as a collaborator on the repo and an author on the package.

@jonschlinkert
Copy link
Contributor Author

jonschlinkert commented Jan 28, 2016

It feels like an overreach

yeah no problem. It's it bit heavy-handed in retrospect. I'll update the pr.

I've just added you as a collaborator on the repo and an author on the package.

thanks!

@jonschlinkert
Copy link
Contributor Author

I removed the .DS_Store code. I didn't even think about that until you said something, which is strange since I have a tendency to try to over-generalize lol

@tkellen
Copy link

tkellen commented Jan 29, 2016

Hah. I was thinking the same thing--this doesn't seem like something @jonschlinkert would add!

package.json Outdated
Copy link

Choose a reason for hiding this comment

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

any reason in particular you've unlocked the deps for mocha/chai?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bah, sorry I ran an "updater" by accident on package.json, I meant to revert that (and thought I did, so I must have run it twice! fwiw, the reason the updater removed mocha is b/c it's not required anywhere in the project. The chai wildcard is a hard-coded default I added a long time ago for a reason I can no longer remember.

anyway, I have two strong habits that are hard not to do before I push up: run update and run deps (for lint-deps). but I try to only do this on my own projects though. so again, apologies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also I can push up a fix for the deps in a bit. sorry again

Copy link

Choose a reason for hiding this comment

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

no worries

Copy link

Choose a reason for hiding this comment

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

can you give me a ping when ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. please let me know if I missed something. feel free to squash

@jonschlinkert
Copy link
Contributor Author

absolutely, won't take more than a minute - just finishing up an obligation first. It will probably be tomorrow. thanks for pinging me!

@tkellen tkellen closed this in e0e894f Feb 7, 2016
@tkellen
Copy link

tkellen commented Feb 7, 2016

published!

@jonschlinkert
Copy link
Contributor Author

thanks!

@tkellen
Copy link

tkellen commented Feb 7, 2016

thank you!

@tkellen tkellen mentioned this pull request Apr 8, 2017
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