search

Bogdanp / django_dramatiq

A Django app that integrates with Dramatiq.
https://dramatiq.io
Other
347 stars 77 forks source link

Add middleware options configuration #102

Closed ashleybartlett closed 2 years ago

ashleybartlett commented 3 years ago

I have been unable to use the original middleware kwarg injection by subclassing the app to work. So this could be user error.

I first went down the path of delaying intialization, though turns out to be important that it is initialized super early, before coming to this. I fee like the django docs have missed something with when AppConfig.ready() is called. This was the simplest, least messy solution, that is still backwards compatible (not that I could get it to work) that I could come up with.

I've tested this against my own app I am developing, which is why I needed it to work. I tried all but the dict response in the README, though I can't see a reason why it would work. I tried it with a list and number, and the error appeared correctly.

I am unsure how I should treat the previous implementation. It could well be an error on my part, and I wasn't prepared to just strip it out. I am pretty sure the README needs some work. Will probably revisit tomorrow, if someone doesn't already have specific feedback.

This PR was cloned from master, I don't know why I have more things to be merged back?

Bogdanp commented 3 years ago

@ashleybartlett now that https://github.com/Bogdanp/django_dramatiq/pull/103 is merged, do you think this PR is still necessary?

ashleybartlett commented 3 years ago

Maybe not, I'll need to retry the app override method.

In the interim, i've had to subclass the intended middleware objects, and override the init, which has worked pretty well so far.

ashleybartlett commented 2 years ago

It's not needed. It would be a design decision on how you want to handle configuration.