Import django-tidings files #5137
Conversation
akatsoulas
commented
May 20, 2022
akatsoulas
commented
- Update imports
- Use only python3
akatsoulas
force-pushed
the
import-tidings
branch
from
bbc48c1
to
82de706
Compare
akatsoulas
force-pushed
the
import-tidings
branch
2 times, most recently
from
a4d33b4
to
12f8533
Compare
* Update imports * Use only python3 * import tidings migrations * drop django-tidings
akatsoulas
force-pushed
the
import-tidings
branch
from
12f8533
to
b318cef
Compare
| from django.contrib.sites.models import Site | ||
| from django.db import connections, models, router | ||
|
|
||
| from .utils import import_from_setting, reverse |
There was a problem hiding this comment.
Nit: Perhaps for consistency with all of the other kitsune.tidings imports within this sub-package, we should make this an absolute import?
from kitsune.tidings.utils import import_from_setting, reverse
| from celery import task | ||
| from django.conf import settings | ||
| from django.contrib.auth.models import User | ||
| from django.db import connection, transaction | ||
| from sentry_sdk import capture_exception | ||
|
|
||
| # NOTE: This import is just so _fire_task gets registered with celery. | ||
| import kitsune.tidings.events # noqa |
There was a problem hiding this comment.
I was wondering if it might be better to register the tidings Celery tasks in the app config? What if we change the tidings app config to something like:
class TidingsConfig(AppConfig):
name = "kitsune.tidings"
default_auto_field = "django.db.models.AutoField"
def ready(self):
# Register the tidings Celery tasks.
from kitsune.tidings import events, tasks # noqaand then move kitsune.tidings in the order of INSTALLED_APPS such that it's the first of the kitsune apps? Something we can consider later if at all, just thinking out loud.
|
@akatsoulas I had the two thoughts above (which you can do with as you wish), and one more thing. I think we should add the unit tests from https://github.com/mozilla/django-tidings/tree/master/tests as well. Overall, this looks excellent to me. |
