-
Notifications
You must be signed in to change notification settings - Fork 529
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(celery): Queues module producer implementation #3079
Conversation
0a72ae0
to
dd9cd92
Compare
dd9cd92
to
3d6154d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally, from a product perspective I think this looks good. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I get it!
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I get it!
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