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

Add setting to override the name of the tasks.py file. #99

Closed
wants to merge 3 commits into from

Conversation

thebjorn
Copy link

@thebjorn thebjorn commented May 4, 2021

This fixes the issues raised in #98 .

@thebjorn
Copy link
Author

thebjorn commented May 4, 2021

The test failures seem related to db-locking issues (I don't think my changes could have caused the failures, nor would I know how to fix them...)

@@ -10,6 +10,8 @@
from django.core.management.base import BaseCommand
from django.utils.module_loading import module_has_submodule

#: The name of the task module
DRAMATIQ_AUTODISCOVER_MODULES = getattr(settings, 'DRAMATIQ_AUTODISCOVER_MODULES', ('tasks',))
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: these need to be double quotes to pass the linter.


return tasks_modules

def _is_package(self, module):
module_path = getattr(module, "__path__", None)
return module_path and os.path.isdir(module_path[0])
try:
return module_path and os.path.isdir(module_path[0])
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better if this keeps failing rather than making it swallow all exceptions. It may be safe to rewrite this entire method to just

def _is_package(...):
  return hasattr(module, "__path__")

quoting the docs:

By definition, if a module has a __path__ attribute, it is a package.

continue

try:
imported_module = importlib.import_module(module)
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to let this throw an exception, as before.

There was also a bug that the `except ImportError` hid, that is now
fixed by doing the work of finding tasks modules during the iteration
over app configs.
@Bogdanp
Copy link
Owner

Bogdanp commented Jun 14, 2021

Thanks! I've squashed and merged this.

@Bogdanp Bogdanp closed this Jun 14, 2021
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.

3 participants