search

Bogdanp / django_dramatiq

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

Making the auto discover module name configurable #97

Closed BradleyKirton closed 3 years ago

BradleyKirton commented 3 years ago

Hi there, I would like to find out what your view is on making the python module name in which tasks reside configurable.

I see from the code that it will always need to lookup tasks in "tasks.py" so that django_dramatiq's own internal tasks are discovered. Given this I can think of a couple ways to extend the task list discovered by the "discover_tasks_modules" function in the "django_dramatiq.management.commands.rundramatiq" module.

  1. Extend "django_dramatiq" to support a user providing a sequence of additional sub module names. The user could for example provide a "DRAMATIQ_AUTODISCOVER_MODULES" setting which could be used in addition to the current default "tasks" to generate the discovered tasks list. So for example if a user specifies DRAMATIQ_AUTODISCOVER_MODULES = ("services", ) "django_dramatiq" would lookup all the "tasks" and "services" sub modules.
  2. Just extend the rundramatiq command in my own project and append tasks module list generated by "discover_tasks_modules". This option will require no change to "django_dramatiq" package

I am happy to override the "rundramatiq" command in my own project but would like to hear your thoughts option 1 above.

Bogdanp commented 3 years ago

This seems like a good idea to me and it should probably be combined with #99. Probably the right approach would be for DRAMATIQ_AUTODISCOVER_MODULES to completely override the modules, so your example would end up as

DRAMATIQ_AUTODISCOVER_MODULES = ("tasks", "services")

so we can support both your and @thebjorn's use cases.

thebjorn commented 3 years ago

It would be easy to modify #99 to handle this case. Insert a second for-loop over DRAMATIQ_AUTODISCOVER_MODULES between lines 156/157, and the except ImportError on L170 will take care of the rest.

thebjorn commented 3 years ago

I've updated PR #99 to support this use case, with the semantics in the comment from @Bogdanp. I used a list in the documentation, since that will lead to fewer support incidents with one-element tuples.

BradleyKirton commented 3 years ago

Hi @Bogdanp and @thebjorn, thank you both :)

Please let me know if there is anything I can do to help out.

thebjorn commented 3 years ago

@Bogdanp is there anything more I can do to get #99 merged?

Bogdanp commented 3 years ago

@thebjorn sorry, I've been busy, but I'll try to take a look at this again during the weekend.