feat(celery): Queues module producer implementation#3079
feat(celery): Queues module producer implementation#3079szokeasaurusrex merged 2 commits intomasterfrom
Conversation
0a72ae0 to
dd9cd92
Compare
dd9cd92 to
3d6154d
Compare
edwardgou-sentry
left a comment
There was a problem hiding this comment.
tested locally, from a product perspective I think this looks good. Thanks!
antonpirker
left a comment
There was a problem hiding this comment.
Just one question, but otherwise looks very good!
| def sentry_publish(self, *args, **kwargs): | ||
| # type: (Producer, *Any, **Any) -> Any | ||
| kwargs_headers = kwargs.get("headers", {}) | ||
| if not isinstance(kwargs_headers, Mapping): |
There was a problem hiding this comment.
If kwargs_headers is a list [("key", "value"), ("k", "v")] then it would be reset to an empty dict?
I am not sure if this list of tuples could happen, but I do not understand this if.
There was a problem hiding this comment.
Basically, what I am trying to do here is to make sure that the get operations (line 453 to 455) are successful.
Mapping (full name collections.abc.Mapping) is the abstract base class that all mapping types in Python inherit from. dict inherits from Mapping, so this isinstance is true for any dictionary, but it would also be possible to create a custom Mapping type, which provides similar functionality to a dict, but which does not inherit from dict. This isinstance also evaluates to True for any of these dict-like data structures.
Regarding your example, isinstance([("key", "value"), ("k", "v")], Mapping) would evaluate to False since list does not inherit from Mapping (it does not have a get method, which all Mapping types have). So, we would replace it with an empty dictionary to ensure that the get calls don't raise an exception.
3d6154d to
de047fe
Compare
| def sentry_publish(self, *args, **kwargs): | ||
| # type: (Producer, *Any, **Any) -> Any | ||
| kwargs_headers = kwargs.get("headers", {}) | ||
| if not isinstance(kwargs_headers, Mapping): |
de047fe to
4402cf4
Compare
Recently `flake8` started to fail with `N803` errors in some places where it had previously passed. This PR adds `# noqa` comments to suppress these errors. Unblocks #3079
4402cf4 to
3283960
Compare
This comment clarifies a potentially confusing part of the code.
3283960 to
22df82d
Compare
Recently `flake8` started to fail with `N803` errors in some places where it had previously passed. This PR adds `# noqa` comments to suppress these errors. Unblocks getsentry#3079
Depends on #3082
Fixes #3078