Skip to content

Make AsyncRetrying callable (as Retrying class)#253

Merged
mergify[bot] merged 7 commits intojd:masterfrom
ivanprado:patch-1
Sep 16, 2020
Merged

Make AsyncRetrying callable (as Retrying class)#253
mergify[bot] merged 7 commits intojd:masterfrom
ivanprado:patch-1

Conversation

@ivanprado
Copy link
Copy Markdown
Contributor

Not entirely sure, but I would say that this line should be added so that it follows the same structure that Retrying class.

Not entirely sure, but I would say that this line should be added so that it follows the same structure that `Retrying` class.
Copy link
Copy Markdown
Owner

@jd jd left a comment

Choose a reason for hiding this comment

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

Does it make sense to have that BaseRetrying instead?
We should add test if it makes sense.

@ivanprado
Copy link
Copy Markdown
Contributor Author

To give you more context. I'm using AsyncRetrying as:

async def task():
  pass

retrying = AsyncRetrying(...)
await retrying.call(task)

But the sync version would be different (according to the documentation):

def task():
  pass

retrying = Retrying(...)
retrying(task)

Note that call invokation is not required in the later. So, at first sight, I would say that this PR makes sense.

Let me know if you think this makes sense. If this is the case I'll try to implement a unit test.

@jd
Copy link
Copy Markdown
Owner

jd commented Sep 14, 2020

Yeah makes sense to me. I think we assumed it was available on BaseRetrying. Does it make sense to move it there?

@ivanprado
Copy link
Copy Markdown
Contributor Author

Does it make sense to move it there?

@jd I've been thinking about it. My proposal would be to just directly override the __call__ in both Retrying and AsyncRetrying. So instead:

class AsyncRetrying(BaseRetrying):

  async def call(...):
     pass

  __call__ = call

We would have:

class AsyncRetrying(BaseRetrying):
  async def call(...):
    pass

The same for Retrying but without the async.

This would also require changing the line https://github.com/jd/tenacity/blob/master/tenacity/__init__.py#L329 for:
return self(f, *args, **kw)
but I think this is Ok.

What do you think?

@ivanprado
Copy link
Copy Markdown
Contributor Author

@jd I've been updating the proposal.

  • Now all classes extending BaseRetrying are forced to implement __call__.
  • Retrying, AsyncRetrying and TornadoRetying are updated accordingly.
  • All unit tests are updated to use __call__ instead of call.
  • A new unit test was included to test this interface on AsyncRetrying.

Let me know what do you think.

Comment thread tenacity/__init__.py
@mergify mergify Bot merged commit 78a462e into jd:master Sep 16, 2020
@ivanprado
Copy link
Copy Markdown
Contributor Author

Thank you @jd for the review.

asqui added a commit to asqui/tenacity that referenced this pull request Oct 20, 2020
asqui added a commit to asqui/tenacity that referenced this pull request Oct 20, 2020
…Tests fail

Tests in TestInvokeViaLegacyCallMethod begin to fail.

This reverts commit 8ee3d27
mergify Bot pushed a commit that referenced this pull request Oct 22, 2020
* Temporarily Revert "Make AsyncRetrying callable (as Retrying class) (#253)"

This reverts commit 78a462e

* Add test coverage for __call__() interface: Tests passing

* Re-apply "Make AsyncRetrying callable (as Retrying class) (#253)": Tests fail

Tests in TestInvokeViaLegacyCallMethod begin to fail.

This reverts commit 8ee3d27

* Fix Retrying.call() to invoke __call__ correctly and return result

* Fix Retrying.call() to raise DeprecationWarning

* pep8
peterthomassen added a commit to peterthomassen/opsgenie-python-sdk that referenced this pull request Mar 2, 2021
Since tenacity 6.0.0, `tenacity.Retrying.call` is deprecated and generates warning messages. Instead one should be calling the `Retrying` instance itself: `retrying()`. jd/tenacity#253

This new recommended behavior was also previously supported (no breaking change).
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.

3 participants