-
-
Notifications
You must be signed in to change notification settings - Fork 929
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 native delayed delivery API to kombu #2128
Conversation
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.
- Add official documentation.
- Make sure the CI passes.
- Make sure you're rebased on main.
Also, I'm not sure about kombu.native_delayed_delivery
. Shouldn't it be in the RabbitMQ transport layer?
This feature is relevant only for RabbitMQ so it should be in the matching place in the code IMHO.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2128 +/- ##
==========================================
+ Coverage 80.33% 80.50% +0.17%
==========================================
Files 75 76 +1
Lines 9125 9179 +54
Branches 1350 1125 -225
==========================================
+ Hits 7331 7390 +59
- Misses 1600 1601 +1
+ Partials 194 188 -6 ☔ View full report in Codecov by Sentry. |
I thought about it and I prefer to keep the API in a separate module. The pre-commit check is failing for unrelated reasons. |
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.
I thought about it and I prefer to keep the API in a separate module.
I included the example in our list of examples and added a short description which is enough documentation for most users.The pre-commit check is failing for unrelated reasons.
There's no problem leaving it in a separate module, but correct me if I'm wrong, this is a transport layer feature, so it should be in the transport.
This is a cosmetic change.
Apart from that, please add the version string to the docs with v5.5 (Current 5.4.2).
We don't expect any patch release soon so the next minor release will include your new feature.
Besides LGTM, thank you!
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.
yes, Native Delayed Delivery API is to Transport layer, so logically, this should be inside the transport
Thank you for explaining @thedrow , but next time please just ping me here so I’ll review the fixes and approve the PR instead. Please pay attention next time, thank you. |
This PR adds the native delayed delivery API to kombu so that it can be used without Celery if required.
The PR includes an example on how to do so.