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

Allow copy's filter to return a Promise #518

Merged
merged 1 commit into from
Nov 14, 2017
Merged

Allow copy's filter to return a Promise #518

merged 1 commit into from
Nov 14, 2017

Conversation

RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Nov 9, 2017

No description provided.

Copy link
Collaborator

@manidlou manidlou left a comment

Choose a reason for hiding this comment

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

This is good @RyanZim! But, you mean we don't need to check if we have a filter opt at all? I mean calling Promise.resolve without checking whether we need to call it or not, wouldn't affect the perf for the cases that we don't have a filter opt?!

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 9, 2017

@manidlou Good point about the perf; wouldn't have much impact if we're just calling a noop function, but with the extra Promise.resolve layer, that will probably have a perf hit.

@RyanZim RyanZim self-assigned this Nov 9, 2017
@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 14, 2017

Updated & fixed merge conflicts.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 86.07% when pulling 9732252 on async-filter into 03fba97 on develop.

Repository owner deleted a comment from coveralls Nov 14, 2017
Copy link
Collaborator

@manidlou manidlou left a comment

Choose a reason for hiding this comment

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

LGTM.

@RyanZim RyanZim merged commit 935e189 into develop Nov 14, 2017
@RyanZim RyanZim deleted the async-filter branch November 14, 2017 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants