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

convert as_manager hooks to base class hook #2282

Merged
merged 1 commit into from
Jul 27, 2024

Conversation

asottile
Copy link
Contributor

finished my idea from #2281 (comment) !

I was getting hung up on overriding methods in inherited querysets -- now it pulls the return type from the base queryset and inherits from that directly

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

I have one thought: doesn't this only apply if as_manager comes from django.db.models.QuerySet or if as_manager is generated by the plugin?

I just mean that the plugin behaviour is probably undefined if as_manager has been explicitly overridden by a user. And in that case we could just exit. Or what do you think?

It's probably a check between the attribute symbol.plugin_generated and perhaps queryset_info.get_containing_type_info if we want to do this.

I had a quick look here: https://grep.app/search?q=def%20as_manager%28&case=true&filter[lang][0]=Python and none with a runtime overrides as_manager. It's not a critical case in any way, was just a thought

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

Ah, nvm, I found the comment and exit now:

# either a manual `as_manager` definition or this is a deferral pass

@flaeppe flaeppe merged commit 8eeed9b into typeddjango:master Jul 27, 2024
36 checks passed
@asottile asottile deleted the as-manager-added-to-class branch July 27, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants