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

feat: add errorFilter option to bypass incrementing failure stats #281

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

lance
Copy link
Member

@lance lance commented Mar 5, 2019

Refs: #262

Still requires documentation.

@lance lance self-assigned this Mar 5, 2019
@lance lance added the review label Mar 5, 2019
@lance
Copy link
Member Author

lance commented Mar 5, 2019

@RidgeA @ComputerWolf @jahredhope please take a look at the requested feature as implemented and let me know what you think.

Copy link

@jahredhope jahredhope left a comment

Choose a reason for hiding this comment

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

Awesome, this looks like it's solve my problem perfectly. Thanks.

@RidgeA
Copy link

RidgeA commented Mar 5, 2019

@lance
This is exactly what I expecting. Looking forward to a landing of this PR.

I have only one doubt - wouldn't be better to accept as an errorHandler promise/async function as well?
Personally I don't nee this at the moment, but it could be useful in some cases?...

@ComputerWolf
Copy link

@lance, yes, thank you for taking the time to implement this!

@lance
Copy link
Member Author

lance commented Mar 5, 2019

@RidgeA I'm concerned about asynchronous execution here - not sure why exactly, but in my thinking, the errorFilter function should be simple and straightforward, only checking the result of a received error. I can imagine a scenario where you might want to call a remote service to determine if the error should really be considered an error. But at first blush, that sounds overly complex and might indicate that the architecture should be reconsidered.

@RidgeA
Copy link

RidgeA commented Mar 5, 2019

@lance
Totally agree, this is could be redundant.
Anyway, if someone will need this feature - they could open a new issue and make PR to enhance the errorHandler

@lance lance force-pushed the 262-error-filter branch from a3442be to fe078aa Compare March 5, 2019 15:39
@lance lance merged commit 8018012 into master Mar 5, 2019
@lance lance removed the review label Mar 5, 2019
@lance lance deleted the 262-error-filter branch March 5, 2019 15:49
@lance
Copy link
Member Author

lance commented Mar 5, 2019

@RidgeA @ComputerWolf @jahredhope this has been merged. It will be a few weeks before it lands in a release, but you should be able to use it in a package.json now directly from GitHub.

@RidgeA
Copy link

RidgeA commented Mar 5, 2019

@lance
Thanks a lot!

@RidgeA
Copy link

RidgeA commented Mar 13, 2019

Hi @lance,
could you give hint when these changes will be in npm?
My company rules prohibit the use of public packages from git repos directly, only NPM...

Thanks!

@lance
Copy link
Member Author

lance commented Mar 14, 2019

@RidgeA I understand - I will try and push a new version today or tomorrow.

@RidgeA
Copy link

RidgeA commented Mar 14, 2019

@lance, would appreciate, thank you!

@lance
Copy link
Member Author

lance commented Mar 14, 2019

@RidgeA I just published v1.11.0 which includes this change.

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.

5 participants