Skip to content

Conversation

@anaisbetts
Copy link
Contributor

This PR allows the ignore option to be a Function as well as a Regexp

test/basic.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be named differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hwhoops, agree

@malept
Copy link
Member

malept commented Jan 27, 2016

Thanks for the PR! I have no objections to this new feature, does any other contributor have any constructive criticism?

(@paulcbetts, once there's a consensus, could you please squash your commits?)

@kfranqueiro
Copy link
Contributor

My one concern is that specifying ignore as a function, but then requiring it to return false to ignore, feels kind of backwards. (This is primarily awkward because previously the function was internal whereas now it's directly exposed.) I realize the doc update explains that, but I figure it's still apt to throw people.

Given that the code calls the ignore function within the function it actually passes to filter anyway, could we throw ! in front of that call so that ignore as a function could be implemented to return true to ignore instead?

@kfranqueiro
Copy link
Contributor

Also, it seems as if the added test failed on CI?

@anaisbetts
Copy link
Contributor Author

(@paulcbetts, once there's a consensus, could you please squash your commits?)

nbd

Given that the code calls the ignore function within the function it actually passes to filter anyway, could we throw ! in front of that call so that ignore as a function could be implemented to return true to ignore instead?

I'm okay with that

Also, it seems as if the added test failed on CI?

Yeah, I gotta fix that up

@max-mapper
Copy link
Contributor

lgtm! only nit is that name is a reserved keyword so I normally don't use it, but it probably doesn't matter here

malept added a commit that referenced this pull request Jan 28, 2016
Allow ignore to be a Function
@malept malept merged commit 5cb50bb into electron:master Jan 28, 2016
@malept malept added this to the Next major or minor version after 5.2.1 milestone Feb 17, 2016
@malept malept mentioned this pull request Feb 17, 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