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

Custom module import behavior (support for Django) #28

Closed
folz opened this issue Dec 18, 2017 · 7 comments
Closed

Custom module import behavior (support for Django) #28

folz opened this issue Dec 18, 2017 · 7 comments

Comments

@folz
Copy link
Contributor

folz commented Dec 18, 2017

Running monkeytype against a file with Django models or dependencies on Django apps gives me a series of errors.

First, just monkeytype apply package.app.models:

django.core.exceptions.ImproperlyConfigured: Requested setting DEFAULT_INDEX_TABLESPACE, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings.

Second, DJANGO_SETTINGS_MODULE=config.settings.local monkeytype apply package.app.models:

File "package/app/models.py"
django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.

Third, adding

import django
django.setup()

to the top of package.app.models:

File "some/other/models.py"
django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.

What has worked for me is patching monkeytype/cli.py and monkeytype/util.py to add

+   import django
+   django.setup()
    mod = importlib.import_module(module)

Obviously this isn't suitable as-is because it breaks monkeytype on non-Django codebases. However, it did give me the idea to have an ModuleImporter (name WIP) context manager that returns importlib.import_module(module) but may also perform pre-/post-import actions. It would work similarly to TypeRewriter: the default implementation could just import_module, but a DjangoModuleImporter could call django.setup(), and the specific ModuleImporter could be configured in settings.

Thoughts? Happy to submit a PR for this if you think it's the right direction.

@carljm
Copy link
Contributor

carljm commented Dec 18, 2017

Ugh, modules that raise errors at import time :)

Yeah, I think we need some way to inject this kind of setup. I'm not sure about wrapping every call to import_module, though; get_name_in_module could be called many times in the same MonkeyType run, we don't really want or need to call django.setup every time, do we?

We could just add a config method called something like cli_setup() that gets called once at startup of the CLI?

@folz
Copy link
Contributor Author

folz commented Dec 19, 2017

Makes sense, yeah. Opened a PR based on your comment.

@carljm
Copy link
Contributor

carljm commented Dec 20, 2017

Fixed with merge of #29.

@carljm carljm closed this as completed Dec 20, 2017
@lsh-0
Copy link

lsh-0 commented Dec 22, 2017

Using cli_context in the linked fix isn't working for me with django (as per the docs) with this as my monkeytype_config.py file:

from monkeytype.config import DefaultConfig
from contextlib import contextmanager

class MonkeyConfig(DefaultConfig):
    @contextmanager
    def cli_context(self, command):
        import django
        django.setup()
        yield

CONFIG = MonkeyConfig()

I get stuck at django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.. It doesn't look like cli_context is being called.

Instead, what is working is simply having this as my monkeytype_config.py file:

import django
django.setup()

from monkeytype.config import DefaultConfig
class MonkeyConfig(DefaultConfig):
    pass

CONFIG = MonkeyConfig()

@folz
Copy link
Contributor Author

folz commented Dec 22, 2017

Looks like the PR that added cli_context hasn't been released to pypi yet, but the docs are built from master.

monkeytype autoimports monkeytype_config.py, so django.setup() there also gets called before the command runs. If it works, it works, I guess!

@carljm
Copy link
Contributor

carljm commented Dec 22, 2017

Thanks for the report, and sorry for the confusion! I've updated the ReadTheDocs config so the default version of the docs is the last tagged release, instead of the last commit. And I'll make a new release today with cli_context. In the meantime, yeah, doing it at module level in your config file is a reasonable workaround.

@carljm
Copy link
Contributor

carljm commented Dec 22, 2017

Version 17.12.3 is released, with cli_context support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants