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

add get_manager to django-cte #30

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

Conversation

aleontiev
Copy link

@aleontiev aleontiev commented Mar 31, 2021

Hi 👋

I think django-cte is great, but for my library use-case I want to force usage of the CTE manager even if the model has some other custom manager or if it has the default manager (i.e. I want to enable usage of CTEs on models outside of my control)

To help facilitate this, I think it would be nice if the logic for determining the manager were factored into a method on With so that it can be changed in a subclass or with monkey-patching.

Thanks, let me know if this is something I should add tests for

Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

Yes, it would be great if tests were added.

@aleontiev
Copy link
Author

aleontiev commented Apr 2, 2021

@millerdev thanks for your review; I can add a simple unit test that verifies this new method looks up the default manager.
I can also try a more complicated test that e.g. creates a new subclass of With that has a different implementation (similar to my actual use-case I am alluding to). Which would your prefer?

@millerdev
Copy link
Contributor

Both seem like good tests.

"""Get a manager for this model

The model's default_manager will be used by default.
This method may be over-riden to use custom managers

Choose a reason for hiding this comment

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

Suggested change
This method may be over-riden to use custom managers
This method may be overwritten to use custom 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.

3 participants