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

accept methods as callbacks as well #110

Merged
merged 2 commits into from
Jan 15, 2019

Conversation

TomasTomecek
Copy link
Contributor

Do you also want me to update docs?

@TomasTomecek TomasTomecek requested a review from a team as a code owner January 7, 2019 16:06
Copy link
Member

@jeremycline jeremycline left a comment

Choose a reason for hiding this comment

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

It'd be good to make sure the documentation is also updated and there's test coverage for this. If you'd like to do that I'd greatly appreciate it, otherwise I can take care of it in the next couple days before I tag a new release.

@@ -495,7 +495,7 @@ def consume(self, callback, bindings=None, queues=None, exchanges=None):
" or a function."
)
self._consumer_callback = cb_obj
elif inspect.isfunction(callback):
elif inspect.isfunction(callback) or inspect.ismethod(callback):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elif inspect.isfunction(callback) or inspect.ismethod(callback):
elif callable(callback):

Is the least restrictive, simplest approach. I can't think of any reason to not do this and I don't know what I was thinking restricting it to a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at docs for callable:

    Note that classes are callable, as are instances of classes with a
    __call__() method.

The only problem I see is when someone passes a class as a callback which doesn't implement __call__.

In [1]: class C:
   ...:     pass
   ...: 
   ...: 

In [2]: callable(C)
Out[2]: True

In [3]: callback = C

In [4]: message = {}

In [5]: callback(message)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-5-5d33cb4c2f38> in <module>()
----> 1 callback(message)

TypeError: C() takes no arguments

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. The if inspect.isclass(callback) should handle this particular case, but in general at the moment we don't validate that the callback really does accept only 1 positional argument. It'd be nice to do that and I assume Python has a way to inspect that, but it wouldn't be a regression in functionality since we don't handle this nicely now.

@jeremycline jeremycline force-pushed the callback-can-be-method branch from 540c3da to 071a275 Compare January 11, 2019 17:33
@codecov-io
Copy link

codecov-io commented Jan 11, 2019

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #110   +/-   ##
=======================================
  Coverage   97.03%   97.03%           
=======================================
  Files          13       13           
  Lines        1080     1080           
  Branches      149      149           
=======================================
  Hits         1048     1048           
  Misses         22       22           
  Partials       10       10
Impacted Files Coverage Δ
fedora_messaging/api.py 100% <ø> (ø) ⬆️
fedora_messaging/_session.py 96.47% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f59e150...1ac7246. Read the comment docs.

@TomasTomecek
Copy link
Contributor Author

Nicely done, thanks for finishing this.

@abompard
Copy link
Member

I think this is worth an entry in the news folder for the release notes :-)

@jeremycline jeremycline force-pushed the callback-can-be-method branch from 071a275 to 9a6f52a Compare January 15, 2019 15:12
TomasTomecek and others added 2 commits January 15, 2019 14:30
Previously, it was only possible to pass a class that implemented
__call__ or a function. However, there are plenty of scenarios where
this restriction is problematic (see fedora-infra#43) and is not really necessary.
Any callable object will do. This brings the behavior in line with the
documentation for the consume API.

Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
The consume API accepted classes (which it would instantiate), but the
API documentation did not mention this.

Signed-off-by: Jeremy Cline <jcline@redhat.com>
@jeremycline jeremycline force-pushed the callback-can-be-method branch from 9a6f52a to 1ac7246 Compare January 15, 2019 19:30
@jeremycline jeremycline merged commit 7325ebe into fedora-infra:master Jan 15, 2019
@TomasTomecek TomasTomecek deleted the callback-can-be-method branch January 16, 2019 13:17
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