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

Remove JoinManager class #613

Closed
wants to merge 2 commits into from

Conversation

mthuurne
Copy link
Contributor

@mthuurne mthuurne commented Apr 16, 2024

The JoinManager class doesn't provide anything that is not already provided by JoinQueryset.as_manager().

My main motivation for removing JoinManager is that it would be too much work to make JoinManager work properly with type annotations, while as_manager() works out of the box thanks to the django-stubs plugin.

This is a backwards incompatible change, as code using JoinManager or JoinManagerMixin would have to be updated. In most cases, I expect that update to be a single-line change; see the unit test changes for an example.

I don't know whether JoinQueryset is still relevant functionality. It generates SQL as a string and contains custom quote code, which always makes me worried about the potential for injection attacks. Perhaps that class could be removed as well?

It doesn't add anything that an autogenerated manager cannot provide.
@mthuurne mthuurne mentioned this pull request Apr 16, 2024
@mthuurne
Copy link
Contributor Author

The docs (docs/managers.rst) will need updating as well. But before spending more time on it, I'd like to know whether you agree with removing JoinManager.

@foarsitter
Copy link
Contributor

The JoinManagerMixin and JoinManager are there only for convenience it looks like.

I don't like breaking code so maybe we should just ignore them for mypy and remove them in de future?

@mthuurne
Copy link
Contributor Author

I don't like breaking code so maybe we should just ignore them for mypy and remove them in de future?

That would be fine with me.

The main thing I want to avoid is a user getting a worse type checking experience because they're not aware that there is a better supported alternative.

I'll close this PR and open a new one to deprecate JoinManager.

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