Skip to content

Fix dj_database_url required key #7979

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

Merged
merged 2 commits into from
May 29, 2022
Merged

Conversation

XF-FW
Copy link
Contributor

@XF-FW XF-FW commented May 28, 2022

Hey. It's me again.

I've happily installed types-dj-database-url, but it seems I've made a small mistake with the PR from earlier. For reference: #7972 .

Required key is ENGINE and not NAME. This fixes that issue.

A question, I have, you might be able to answer me: I have a project that uses dj-database-url. If I wanted to test these stubs against my project, how would I go about doing so?

Thanks.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

@XF-FW
Copy link
Contributor Author

XF-FW commented May 28, 2022

You're not. That's indeed the case, I had misinterpented the problem, but there's still one:

Assigning, for example, DATABASES["default"] to a call to dj_database_url.config, will set its type to _DBConfig, but Django itself, does not require either NAME or ENGINE:

https://github.com/django/django/blob/0dd29209091280ccf34e07c9468746c396b7778e/django/db/utils.py#L149

My problem is that I have something like this on the settings of my Django project:

import sys

import dj_database_url

# Database
# https://docs.djangoproject.com/en/4.0/ref/settings/#databases
DATABASES = {}
DATABASES["default"] = dj_database_url.config(
    conn_max_age=600, default="postgres://postgres:postgres@localhost/my-app"
)
DATABASES["default"]["OPTIONS"] = {"application_name": "my-app"}

# We don't need a live database to check if there are pending migrations
if_checking_if_has_pending_migrations = {
    "makemigrations",
    "--check",
    "--dry-run",
}.issubset(set(sys.argv))
if if_checking_if_has_pending_migrations:  # pragma: no cover
    DATABASES = {"default": {"ENGINE": "django.db.backends.dummy"}}

Since mypy got its type from dj_database_url, I'm getting this on the last line: error: Missing key "NAME" for TypedDict "_DBConfig".

That's the problem. I think the solution, would be to have everything as optional, as that's what django expects.

Alternatively, I can just set DATABASES to empty dict after the if, as that's the default value, but I like the explicitness and I think the above solution is the correct one.

@XF-FW
Copy link
Contributor Author

XF-FW commented May 29, 2022

I've made the change to make everything optional.

Also, for reference, django-stubs defines DATABASES as Dict[str, Dict[str, Any]]:

https://github.com/typeddjango/django-stubs/blob/43b082850c8ca4e00194165f02be798a0943fb21/django-stubs/conf/global_settings.pyi#L80

I'm beginning, to think, that's the best solution, to make sure it behaves nicely. Especially, since there are more keys missing from what DATABASES[alias] accepts. It's missing, for example, ATOMIC_REQUESTS and AUTOCOMMIT.

What do you think?

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Got it. Thanks for the fix!

If I wanted to test these stubs against my project, how would I go about doing so?

If you want to test changes to these stubs prior to filing a PR, you can make the changes in your local clone of typeshed, and then point mypy towards these stubs by using the MYPYPATH environment variable and setting it to the stubs directory in your local clone of typeshed. If your project's open-source, you could also make a request to have it added to the mypy_primer project :)

@AlexWaygood AlexWaygood merged commit c94fb40 into python:master May 29, 2022
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