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

Adds support for additional kwargs in middlewares (Solves issue #82) #83

Closed
wants to merge 3 commits into from

Conversation

dnmellen
Copy link

@dnmellen dnmellen commented Oct 7, 2020

I'm trying to make the middleware initialization more flexible by adding a hooks system to be able to override the initial kwargs for some middleware during the django_dramatiq app initialization.

The way you would provide this hook would be by overriding django_dramatiq django conf:

If you want to add dramatiq.middleware.GroupCallbacks middleware, the name of the hook would be middleware_groupcallbacks_kwargs. It takes the middleware name and converts it to lowercase (middleware_<middlewarename>_kwargs).

The hook is a classmethod that returns a dict containing the kwargs for that middleware.

from django_dramatiq.apps import DjangoDramatiqConfig, RATE_LIMITER_BACKEND

class MyDjangoDramatiqConfig(DjangoDramatiqConfig):

    @classmethod
    def middleware_groupcallbacks_kwargs(cls):
        return {"rate_limiter_backend": cls.get_rate_limiter_backend()}

I was completely unable to create tests for this feature without changing the current test settings, but these changes are not changing any functionality unless you override the DjangoDramatiqConfig.

@Bogdanp
Copy link
Owner

Bogdanp commented Oct 10, 2020

@dnmellen thanks! I'll dig into this next weekend.

@@ -36,13 +36,12 @@ class DjangoDramatiqConfig(AppConfig):
name = "django_dramatiq"
verbose_name = "Django Dramatiq"

@classmethod
def initialize(cls):
def ready(self):
Copy link
Author

Choose a reason for hiding this comment

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

@Bogdanp I think you have a nice reason to use the @classmethod decorated initialize method. I tried my branch in my django project and some things were broken because the ready() method runs too late. I'm going to modify this to keep using your initialize method.

Copy link
Author

Choose a reason for hiding this comment

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

BTW with the latest changes, I could use the GroupCallbacks middleware successfully in my project.

@Bogdanp
Copy link
Owner

Bogdanp commented Oct 24, 2020

Thanks again. This looks good, but would you mind adding an example to the README? An adapted version of your OP here would be fine. If not, I can take care of it next week.

@dnmellen
Copy link
Author

No problem! I can change the README.

@dnmellen
Copy link
Author

@Bogdanp I added some documentation to the README. Feel free to complete or change anything you want for better clarification.

Thanks!

@Bogdanp
Copy link
Owner

Bogdanp commented Nov 1, 2020

Thanks again. I made some minor changes on top and merged your changes with rebase. I'll cut a release in the next couple of weeks.

@Bogdanp Bogdanp closed this Nov 1, 2020
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.

None yet

2 participants