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

Proxy query attribute. #87

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

Conversation

ride90
Copy link

@ride90 ride90 commented Jan 11, 2022

Hi @clokep ,

Please, take a look at this PR.
I saw issues with Django admin (for example #65) and with other packages using querysetsequence . All these issues have pattern: 'QuerySetSequence' object has no attribute 'query'.
This PR should fix it and shouldn't break anything.

Please, let me know wdyt 🙏 ?

@clokep clokep self-requested a review January 11, 2022 16:13
@ride90
Copy link
Author

ride90 commented Jan 12, 2022

Hi @clokep ,

Do you think you'll have time to take a look?

@clokep
Copy link
Owner

clokep commented Jan 12, 2022

Do you think you'll have time to take a look?

Hopefully soon, but I have been rather busy recently. We removed the query property from QuerySetSequence at some point in the past so will need to check why / when that was done.

Weirdly the QuerySet docs don't even state this as a publicly available property.

@ride90
Copy link
Author

ride90 commented Jan 12, 2022

Thank you!

Yes, that's weird, for some reason Django admin uses .query property even in 4.0..

@ride90
Copy link
Author

ride90 commented Jan 18, 2022

Hi @clokep ,
Sorry for bothering you, but will you have a time to check it anytime soon?

Thanks

@clokep
Copy link
Owner

clokep commented Jan 18, 2022

Hi @clokep , Sorry for bothering you, but will you have a time to check it anytime soon?

I hope to get to it soon, but have been busy as I mentioned above. I maintain this package in my spare time and am not paid to do so; that, unfortunately, puts it below many of my other professional and personal obligations.

@clokep
Copy link
Owner

clokep commented Feb 11, 2022

Can you add some tests to this?

Do you know if Django admin using these values is considered "wrong" or if they're meant to be public APIs? I can't find any information about what the .query attribute is (from reading the source it is a Query object, but that seems completely undocumented in the Django docs?)

Copy link
Owner

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Marking as reviewed, please see my previous comment!

@ride90
Copy link
Author

ride90 commented Feb 14, 2022

Hi @clokep , I will provide tests when I have time. Approx 1-2 weeks.
Thank you for your review.

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.

None yet

2 participants