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

Make as_manager method use proper generic #327

Closed
wants to merge 2 commits into from

Conversation

silviogutierrez
Copy link

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix!

@@ -41,7 +41,7 @@ class _BaseQuerySet(Generic[_T], Sized):
hints: Optional[Dict[str, models.Model]] = ...,
) -> None: ...
@classmethod
def as_manager(cls) -> Manager[Any]: ...
def as_manager(cls) -> Manager[_T]: ...
Copy link
Member

Choose a reason for hiding this comment

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

Can you please also test it? We have a lot of bugs in QuerySet / Manager topics.
And we want to be sure it is as stable as possible.

@lithammer
Copy link

Possible duplicate work in #290.

@silviogutierrez
Copy link
Author

@sobolevn : forgive me if I missed it, but is there a guide to running or developing locally? I'm trying to run tests and doing it through CI passes really takes a while. Got a lot of other fixes too that I'd love to contribute back.

I tried replicating the Travis steps but no luck.

An example here: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/unit-tests/#running-the-unit-tests

@sobolevn
Copy link
Member

sobolevn commented Mar 13, 2020

Yes, we have a really short guide: https://github.com/typeddjango/django-stubs/blob/master/CONTRIBUTING.md

It is not perfect and I am open for any feedback!

@RJPercival
Copy link
Contributor

Has this been abandoned?

@sciyoshi
Copy link

We are waiting for this change as well as it is blocking us upgrading to Python 3.9. Is this still only waiting on tests @sobolevn?

@sciyoshi
Copy link

It does look like the PR includes a test already - I'm happy to help if there's anything needed to move this forward.

@sobolevn
Copy link
Member

I would love to merge this after the CI is fixed.

@sciyoshi
Copy link

After investigating a bit more - looks like this simple approach doesn't quite work and it will probably need some of the more advanced machinery from #272 or #290. I'd recommend closing this PR in favor of one of the other two unless @silviogutierrez has some further input on it.

@silviogutierrez
Copy link
Author

@sciyoshi : yea, #272 and #290 are much better solutions. Thanks all.

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.

Make as_manager() in queryset generic instead of using Any
5 participants