Skip to content

Remove unintentional re-exports #1309

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 1 commit into from
Feb 10, 2023

Conversation

intgr
Copy link
Collaborator

@intgr intgr commented Jan 5, 2023

Follow-up from #1308, applying the same to the whole django-stubs code base.

Stub files with from ... import SomeSymbol as SomeSymbol are explicit re-exports (https://typing.readthedocs.io/en/latest/source/stubs.html#imports)

Django does employ intentional re-exports to make imports shorter and more convenient (usually in __init__.py files). But many of these re-exports were under longer module paths. And these unintentional re-exports are likely to be less stable over Django releases. E.g.

# Instead of:
from django.contrib.admin.exceptions import SuspiciousOperation
from django.db.backends.oracle.base import BaseDatabaseWrapper
# Use:
from django.core.exceptions import SuspiciousOperation
from django.db.backends.base.base import BaseDatabaseWrapper

Now mypy will warn users when using these re-exports.

Also cleaned up lots of unused imports in our codebase in the files that I touched.

@intgr intgr added the help wanted Extra attention is needed label Feb 7, 2023
Copy link
Contributor

@ljodal ljodal left a comment

Choose a reason for hiding this comment

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

I see that you've removed a bunch of imports that are actually present in the Django source code, meaning that mypy will not emit "has not attribute" errors rather than warn about re-exports. Was that intentional? I find it hard to imagine someone actually wants to use any of these, but it might still be nice to match actual runtime behavior and leave the imports in-place with # noqa comments?

Comment on lines -5 to -7
from django.contrib.postgres.validators import ArrayMaxLengthValidator as ArrayMaxLengthValidator
from django.contrib.postgres.validators import ArrayMinLengthValidator as ArrayMinLengthValidator
from django.core.exceptions import ValidationError as ValidationError
Copy link
Contributor

Choose a reason for hiding this comment

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

These appear to be present here in the Django source code, so shouldn't they be kept with a # noqa: F401 comment?

@@ -1,6 +1,6 @@
from typing import Any

from django.utils.connection import BaseConnectionHandler, ConnectionProxy
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, this import appears to be present in the source code, so shouldn't this also have a # noqa: F401 comment instead?

@intgr
Copy link
Collaborator Author

intgr commented Feb 8, 2023

I see that you've removed a bunch of imports that are actually present in the Django source code, meaning that mypy will not emit "has not attribute" errors rather than warn about re-exports. Was that intentional?

Good point. It was intentional, but I didn't fully consider that this changes the error message for users.

The difference is error: Module "django.core.cache" has no attribute "ConnectionProxy"

Compared to error: Module "django.core.cache" does not explicitly export attribute "ConnectionProxy"

The question is whether the clarity in error message is sufficient outweigh the maintenance of these legacy imports. I agree the latter is more likely to nudge the user in the direction of "Oh OK, I need to find another place to import this from"

But on the other hand, they were never a stable part of Django's public API and it was an accident they were there. It's inconsistent as well, we don't carry the majority of imports present in Django's own codebase, these removals are exceptions.

Personally I'd prefer not to keep them around, the whole idea of type stubs is to accurately describe the public API surface, and sometimes new mypy errors are expected with stubs releases, as we inch closer to accurately describing the public API surface.

@intgr intgr force-pushed the remove-unintentional-reexports branch from c57f2eb to 57029e6 Compare February 8, 2023 14:10
Copy link
Contributor

@ljodal ljodal left a comment

Choose a reason for hiding this comment

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

Alright, sounds good to me. Looking through the list here I'd hope the number of users affected by this is low 🤞

@intgr
Copy link
Collaborator Author

intgr commented Feb 10, 2023

👍 I'll try to remember to mention this in release notes.

@intgr intgr merged commit cf8c0a6 into typeddjango:master Feb 10, 2023
@intgr intgr deleted the remove-unintentional-reexports branch February 10, 2023 10:55
@intgr
Copy link
Collaborator Author

intgr commented Feb 22, 2023

Added this to release notes:

Some re-exports that were unintentional have been removed. If you now get errors
like error: Module "django.core.cache" has no attribute "ConnectionProxy",
you probably need to change the module you are importing ConnectionProxy from.
These re-exports were never intentional and are not stable across Django versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Development

Successfully merging this pull request may close these issues.

2 participants