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

add filter option to retry() and retryable(). PR for #1256. #1261

Merged
merged 4 commits into from Aug 8, 2016
Merged

add filter option to retry() and retryable(). PR for #1256. #1261

merged 4 commits into from Aug 8, 2016

Conversation

bojand
Copy link
Contributor

@bojand bojand commented Aug 2, 2016

  • add filter option to retry() and retryable() to allow for error filtering and control of retry flow. Resolves error filter for retry and retryable #1256.
  • Added comment regarding the new option. Do I need to run the script to update the generated documentation?
  • The new option is called errorFilter.
  • Added tests. Lint and all existing tests pass when I run with npm test.

Fixes #1256 - megawac

@@ -19,6 +19,10 @@ import constant from 'lodash/constant';
* * `interval` - The time to wait between retries, in milliseconds. The
* default is `0`. The interval may also be specified as a function of the
* retry count (see example).
* * `filter` - Synchronous function that is invoked on erroneous result with the
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think filter is the right term for this. Perhaps continueOperation or something? /cc @hargasinski @aearly
Also this should be documented as optional

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 for continueOperation. I'd also be open to errorFilter from the issue,

Also, you don't need to run the script to generate the documentation. Just make sure the tests pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't fan of filter either. Most of the do / while type of functions within async use test... maybe something similar to that? continueTest?

Copy link
Contributor Author

@bojand bojand Aug 3, 2016

Choose a reason for hiding this comment

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

Missed the comment somehow. Thanks for the feedback. I will change it to continueOperation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

errorFilter/errorTest/shouldError/onError/shouldContinueOnError

I think continueOperation is a bit misleading, because it's not something you do when you continue, it's a test/callback to see if you should continue or not.

Cache invalidation and naming things....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like errorFilter or errorTest. Another option I suppose is retryTest ?

…t documentation and fixed code based on PR feedback.
* erroneous result with the the error. If it returns `true` the retry attempts
* will continue, if the function returns `false` the retry flow is aborted
* with the current attempt's error and result being returned to the final
* callback. Invoked with (err).
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't it invoked with (error, ...any other arguments)

});

setTimeout(function () {
}, 15);
Copy link
Collaborator

Choose a reason for hiding this comment

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

qu'est-ce que?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this timeout for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had used an existing test which had it; and I didn't notice it. Not sure why it's there to be honest. In the latest commit I removed it from the existing test and the new one. Tests pass locally for me, and in CI it seems.

@megawac megawac merged commit ff65da5 into caolan:master Aug 8, 2016
@megawac
Copy link
Collaborator

megawac commented Aug 8, 2016

Thanks!

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.

None yet

4 participants