typeddjango / django-stubs

PEP-484 stubs for Django
MIT License
1.61k stars 452 forks source link

`settings.py` is executed locally #458

Open WhyNotHugo opened 4 years ago

WhyNotHugo commented 4 years ago

Bug report

What's wrong

This plugin attempts to execute settings.py. I'm using 12factor, where I use about 15 environment variables to configure my app. The defaults for these make the crash to indicate a configuration error (e.g.: these variables must be set).

Re-architecting my entire application to support this seems like a wrong direction: I don't want to add complexity to critical production code just to set up better type-linting (and I don't think that's a good direction generally).

My editor runs mypy when I read files too, so executing them for linting is dangerous since it implies I cannot inspect untrusted code before it gets run.

How is that should be

On (partial) solution would be to allow configuring environment variables that are passed on when settings.py is executed.

Another would be to build an AST from settings.py, and infer types from there. This sounds like it's the proper approach to type-check properly

System information

sobolevn commented 4 years ago

Another would be to build an AST from settings.py, and infer types from there. This sounds like it's the proper approach to type-check properly

Sadly, this is not possible. There are a lot of thing we need from settings.py like INSTALLED_APPS (it can be a dynamic array), URL_CONF, etc, etc

On (partial) solution would be to allow configuring environment variables that are passed on when settings.py is executed

This also seems like an overkill to me. Maybe you can create settings_mypy.py with values that you need? And use it during the type-checking?

WhyNotHugo commented 4 years ago

Maybe you can create settings_mypy.py with values that you need? And use it during the type-checking?

That's not unreasonable. But wouldn't I need to manually keep that in sync when adding INSTALLED_APPS (and other variables)?

WhyNotHugo commented 4 years ago

Sorry, I'm asking the wrong questions. I guess the right question is: What's the minimal fields that are required on the settings file for the plugin to work?

sobolevn commented 4 years ago

What's the minimal fields that are required on the settings file for the plugin to work?

We use full django.setup() here: https://github.com/typeddjango/django-stubs/blob/master/mypy_django_plugin/django/context.py#L70

So, it needs all fields django needs. AFAIK, we don't require any extra fields to be set.

WhyNotHugo commented 4 years ago

Thanks! I'll try to figure out a minimal set and will try to push a PR with docs for others who are curious :)

WhyNotHugo commented 4 years ago

I've tried to create a settings file with just the INSTALLED_APPS, SECRET_KEY, and a bunch of dummy settings. However, that's an endless rabbithole.

My application uses many python packages which, at runtime, require certain dependencies (including C libraries, and system libraries). I can't install those on my system; some are not packaged, other have incompatible versions, and other conflict with system packages. I tried a bit, but I was getting to close to breaking my OS, so that's definitely not the way to go.

Even if I took a couple of days to mess with my OS setup to make it work, the next person working on it has to do the same thing for their specific OS, and that's just not feasible. Running django.setup() on my system is simply far too complex.

If seems that the way to run type-checking is via code analysis, not code execution. E.g.: I can run plain mypy on code that won't run on my system just fine.

sobolevn commented 4 years ago

If seems that the way to run type-checking is via code analysis, not code execution. E.g.: I can run plain mypy on code that won't run on my system just fine.

Mypy does not have to figure out what apps django has and what settings and models there are. So, it is possible for it to be static only. However, we cannot work without running django.setup.

Analyzing pure ast is way too complex. We have tried this before 1.4. And it didn't work.

I am sorry to hear that django-stubs does not work for your use-case, but that's the best we can do.

WhyNotHugo commented 4 years ago

I honestly haven't been able to figure out how ti get django-stubs to work for any project (aside from a very small library where I'm blocked by #459).

I understand using an ast is complex, but the alternative simply doesn't work, and I don't see it every working. Attempting to do type-checking through code-execution clearly has too many limitations, and ultimately, will just leave many projects in the dark.

The main fault is, IMHO, assuming that a developers' local environment is the same as the target environment. This is simply not usually true, especially for Django applications which will ultimately run in a dedicated environment, not on a local system.

How would you advise running django-stubs given that django.setup() attempts to load libraries that cannot run on my local OS?

WhyNotHugo commented 4 years ago

I wonder if a compromise might be possible: would it be possible to gracefully fall back when django.setup() cannot be run, and only apply type hints to things that do no rely on it?

For example, this sort of scenario does not require anything specific to a settings.py, and it's be a huge benefit to avoid having to explicitly annotate these (it just hurts readability so much!).

class Person(models.Model):
    name = models.CharField(...)
    # ^ automatically infer that this is of type str.
    name = models.IntegerField(...)
    # ^ automatically infer that this is of type int.
danjac commented 3 years ago

This is precisely the issue I ran into here: https://github.com/typeddjango/django-stubs/issues/613. My Django projects work in Dockerized containers, and this is increasingly the standard for teams working on non-trivial applications. The only way I was able to get django-stubs to work was to run it inside a container - while this is OK for CI and local development (I do the same for the test suite, after all) it means that django-stubs cannot be integrated with external tooling such as pre-commit hooks or an IDE such as VSCode, limiting its usefulness.

Ultimately I gave up on django-stubs as the contortions required to get it working were too great to be worth the additional type-checking safety. Other teams may have more stringent requirements, but it would probably be good to highlight this fundamental issue in the README that you have to run django-stubs inside a dedicated environment.

dhruvkb commented 12 months ago

django-stubs cannot be integrated with external tooling such as pre-commit hooks

This got me as well. I am trying to type-hint a Django project while running mypy as a pre-commit hook. To get django-stubs to work I would need to add almost all my dependencies as additional_dependencies in the mypy pre-commit hook which is untenable.

WhyNotHugo commented 12 months ago

On Tue, 28 Nov 2023, at 15:01, Dhruv Bhanushali wrote:

django-stubs cannot be integrated with external tooling such as pre-commit hooks

This got me as well. I am trying to type-hint a Django project while running mypy as a pre-commit hook. To get django-stubs to work I would need to add almost all my dependencies as additional_dependencies in the mypy pre-commit hook which is untenable.

I worked around this by simply running mypy from my development environment instead of having mypy install another copy:

  - repo: local
    hooks:
      - id: mypy
        name: mypy
        language: system
        entry: mypy
        types_or: [python, pyi]
        args: ["--scripts-are-modules"]
        require_serial: true

Note that mypy also requires all of your project's dependencies too, so this also avoids having to keep a duplicate list of all the dependencies (and having to keep them in sync).

intgr commented 11 months ago

To get django-stubs to work I would need to add almost all my dependencies as additional_dependencies in the mypy pre-commit hook which is untenable.

Another alternative is to run mypy as a regular CI job, not as part of pre-commit.

dhruvkb commented 11 months ago

I went the language: system way, it works well enough for my usecases 👍. I haven't set up a CI pipeline and setting it up would be more work than just running up mypy as a pre-push hook.