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

Tweak Meta.base_manager_name check message #236

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adamchainz
Copy link
Collaborator

No description provided.

@adamchainz
Copy link
Collaborator Author

@seporaitis @tolomea does this seem good to you?

@tolomea
Copy link
Owner

tolomea commented Apr 6, 2023

I'm unsure if this is a good idea. The default and base manager stuff rapidly gets quite confusing for people.

In the case of auto prefetch you really want both of them and probably actually all managers to either be or inherit from the auto prefetch one.

Focusing on the base manager. I think there's two ways people end up with the wrong manager.
1: the Meta inheritance chain is broken and they haven't replicated the setting down, common.
2: they are deliberately setting their own base manager, rare.

Checking the name is going to be annoying for the case 2 people, but they are also the best equipped to disable the check.

As for the case 1 people you seem to be suggesting that we should push them to replicate the setting down, which I guess works.
Personally I'm a strong believer in being thorough and consistent about Meta inheritance.
But maybe prefetch doesn't need to get involved in teaching these things.

@adamchainz
Copy link
Collaborator Author

Django does meta inheritance automatically, but some of the time it doesn't work. Proxy models and intermediate abstract models seem to lose the inheritance of the attr. Perhaps I need to do more research into why, I kinda just adjusted the check for the issue @seporaitis reported.

But also you're right, we shouldn't check for the exact attr but the correct inheritance, on all managers.

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.

2 participants