Added dynamic class hook for `from_queryset` manager #427

kszmigiel · 2020-07-16T17:51:06Z

No description provided.

mypy_django_plugin/transformers/managers.py

mypy_django_plugin/lib/sem_helpers.py

mypy_django_plugin/transformers/managers.py

mkurnikov · 2020-07-19T21:21:39Z

mypy_django_plugin/lib/helpers.py

    @abstractmethod
    def create_new_dynamic_class(self) -> None:
        raise NotImplementedError

+class DynamicClassFromMethodCallback(DynamicClassPluginCallback):
+


Add class attribute callee here, it should be certain that it's a MemberExpr. Also, add assert with the type in the __init__ so that it would fail if this subclass is used incorrectly. It will allow a bit nicer autocompletion and maybe will save you an assert somewhere in subclasses.

Adding an assert in __init__ makes like no sense, we don't have DynamicClassDefContext available up there (only NewSemanalDjangoPlugin instance passed in ManagerFromQuerySetCallback(self) in get_dynamic_class_hook. I will add an assert in __call__ though :)

Yes, in __call__, my bad:)

mkurnikov · 2020-07-19T21:24:46Z

mypy_django_plugin/transformers/managers.py

+
+        method_type = method_node.type
+        if not isinstance(method_type, CallableType):
+            if not self.semanal_api.final_iteration:


defer_till_next_iteration too?

@mkurnikov will if not isinstance(method_type, CallableType) and not self.defer_till_next_iteration() work? I'm not 100% sure if I get it right how deferring works

If it's not isinstance(CallableType), which type it is? Placeholder?

It might be Overloaded, CallableType or None actually

mypy_django_plugin/transformers/managers.py

mkurnikov · 2020-07-19T21:34:04Z

mypy_django_plugin/transformers/managers.py

+                method_node.arguments[1:]):
+            bound_arg_type = self.semanal_api.anal_type(arg_type, allow_placeholder=True)
+            if bound_arg_type is None and not self.semanal_api.final_iteration:
+                self.semanal_api.defer()


Same thing as above, defer_till_next_iteration or/and exception raise, instead of just return.

mypy_django_plugin/transformers/managers.py

mkurnikov · 2020-07-21T11:54:34Z

mypy_django_plugin/transformers/managers.py

+
+        base_manager_info = self.callee.expr.node
+
+        if base_manager_info is None and not self.defer_till_next_iteration(reason='base_manager_info is None'):


I don't think this reason is useful. base_manager_info is None means something, it should be added as the reason.
How could it happen that TypeInfo is None here?

Also, let's see what type self.callee.expr is. It's an expression on the left side of the a.b. It could be get_manager_class().from_queryset() (which will be CallExpr as self.callee.expr, not a RefExpr), so there's no node attribute available, and so forth.
If it's not a RefExpr, a good idea would be to fallback to Any here.

@kszmigiel I still want you to add proper error string here with an explanation of the conditions of why it could be None.

… this one

…orrectly

sobolevn · 2020-08-03T17:32:46Z

mypy_django_plugin/lib/helpers.py

@@ -115,7 +115,7 @@ def copy_method_to_another_class(
                self_type=self_type)
            return

-        method_type: CallableType = method_node.type
+        method_type: CallableType = method_node.type # type: ignore


cast maybe?

Applied, works, thanks for the tip 👍

mypy_django_plugin/lib/helpers.py

sobolevn · 2020-08-05T11:11:20Z

django-stubs/contrib/admin/options.pyi

@@ -135,7 +135,7 @@ class ModelAdmin(BaseModelAdmin):
    delete_selected_confirmation_template: str = ...
    object_history_template: str = ...
    popup_response_template: str = ...
-    actions: Sequence[Callable[[ModelAdmin, HttpRequest, QuerySet], None]] = ...
+    actions: Sequence[Union[Callable[[ModelAdmin, HttpRequest, QuerySet], None], str]] = ...


I guess that this PR has unrelated changes from other PRs. Is that the case, @kszmigiel?

Yes, I've updated stub files to the master version, I can revert it if needed (this commit: b032a3d)

The proper way to do it is to rebase this PR based on current master

👍 It'll be quite a challenge for me, but I'll try

kszmigiel · 2020-08-05T13:30:25Z

@sobolevn do you have any idea why this test I've added don't pass? I added

from .models import ModelA

reveal_type(ModelA.objects.do_something())

in views.py of reproduction repository provided by @danifus and it works just fine

My Django version is 3.1b1 and Mypy is 0.782 if that helps

sobolevn

LGTM! 👍

Can we merge it? @kszmigiel

sobolevn · 2020-08-06T17:01:43Z

@kszmigiel looks like it is passing now.

kszmigiel · 2020-08-06T17:11:32Z

@sobolevn I think we're ready to merge, but I would like to hear @mkurnikov opinion first. I know there are a few fragments I need to cleanup, but I think I can do this along with writing docs and comments after the rebase to master.

BTW how about getting readthedocs.io page for docs? I can take care of writing the docs as the task for next week - we lack a lot of them, and I could use a short break from coding too.

mkurnikov · 2020-08-07T13:25:16Z

mypy_django_plugin/lib/helpers.py

+
+        assert ctx.call.callee is not None
+        if not isinstance(ctx.call.callee, MemberExpr):
+            # throw error?


Add assert here, instead of isinstance+return.

Done, I will include this in next PR

mkurnikov · 2020-08-07T13:26:54Z

mypy_django_plugin/transformers/managers.py

+
+        base_manager_info = self.callee.expr.node
+
+        if base_manager_info is None and not self.defer_till_next_iteration(reason='base_manager_info is None'):


@kszmigiel I still want you to add proper error string here with an explanation of the conditions of why it could be None.

mkurnikov · 2020-08-07T13:36:06Z

test-data/typecheck/managers/querysets/test_from_queryset.yml

@@ -0,0 +1,236 @@
+-   case: from_queryset_with_base_manager


According to the test, there has to be some kind of magic somewhere which adds MyModel as a generic argument for NewManager. Could we get rid of that?
So, what I'm trying to say is that tests are a bit wrong. from_queryset line should be

class ModelQuerySet(models.QuerySet[MyModel]): ... NewManager = BaseManager[MyModel].from_queryset(ModelQuerySet)

and current line should error out with Need type annotation for like it's done here https://github.com/typeddjango/django-stubs/blob/newsemanal-progress/test-data/typecheck/managers/test_managers.yml#L128
and NewManager in the current setup should give Any as a generic argument, see https://github.com/typeddjango/django-stubs/blob/newsemanal-progress/test-data/typecheck/managers/test_managers.yml#L106

Could be done in a separate PR though.

kszmigiel · 2020-08-07T17:58:45Z

🎉

Kacper Szmigiel added 3 commits July 16, 2020 04:09

add callback class for from_queryset()

89a5797

helpers

6d8a439

typos

0ca8abc

kszmigiel requested a review from mkurnikov July 16, 2020 17:51

method for copying methods

51207ba

sobolevn reviewed Jul 17, 2020

View reviewed changes

mypy_django_plugin/transformers/managers.py Outdated Show resolved Hide resolved

mkurnikov requested changes Jul 19, 2020

View reviewed changes

cleanp after 1st CR

8af158d

mkurnikov requested changes Jul 19, 2020

View reviewed changes

cleanup after 2nd CR, exceptions instead of returns

8ca96f8

mkurnikov requested changes Jul 21, 2020

View reviewed changes

Kacper Szmigiel added 18 commits July 24, 2020 00:21

fallback to Any if can not resolve from_queryset Manager

98ca0cc

implemented AddManagers.run_with_model_cls() from master

4102a7c

tests for from_queryset

48feb3f

remove print

fae8316

flak8 cleanup

dabdfa1

update stubs to master version

b032a3d

reverted to old way of creating typeinfo, probably refactor needed on…

745d00f

… this one

fix 2 of failing tests

354e9f4

fix 3rd test

78c8b93

fix the last test

d8b2427

isort

12a313b

fix first mypy plugin code error

1883270

another try to fix mypy for plugin code errors

30e53d7

fix isort

203eeaf

added type ignore comments in places where mypy doesnt resolve type c…

e43fd2b

…orrectly

flake8

5eba681

error message formatted properly

dae217a

unused type ignore comment

725377d

sobolevn reviewed Aug 3, 2020

View reviewed changes

use of cast instead of type ignore

1ae7eca

sobolevn reviewed Aug 5, 2020

View reviewed changes

Kacper Szmigiel added 2 commits August 5, 2020 14:53

fix issue typeddjango#438 manager method is forward reference

43ef5a9

typo in test

a6cb6f3

fixed condition for defering

164f83d

sobolevn approved these changes Aug 6, 2020

View reviewed changes

mkurnikov requested changes Aug 7, 2020

View reviewed changes

mkurnikov merged commit 50372fa into typeddjango:newsemanal-progress Aug 7, 2020

kszmigiel mentioned this pull request Aug 26, 2020

GSoC 2020 Summary #450

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added dynamic class hook for `from_queryset` manager #427

Added dynamic class hook for `from_queryset` manager #427

kszmigiel commented Jul 16, 2020

mkurnikov Jul 19, 2020

kszmigiel Jul 20, 2020

mkurnikov Jul 21, 2020

mkurnikov Jul 19, 2020

kszmigiel Jul 19, 2020 •

edited

Loading

mkurnikov Jul 20, 2020

kszmigiel Jul 20, 2020

mkurnikov Jul 19, 2020

mkurnikov Jul 21, 2020

mkurnikov Aug 7, 2020

sobolevn Aug 3, 2020

kszmigiel Aug 5, 2020

sobolevn Aug 5, 2020

kszmigiel Aug 5, 2020

sobolevn Aug 5, 2020

kszmigiel Aug 5, 2020 •

edited

Loading

kszmigiel commented Aug 5, 2020

sobolevn left a comment

sobolevn commented Aug 6, 2020

kszmigiel commented Aug 6, 2020

mkurnikov Aug 7, 2020

kszmigiel Aug 11, 2020

mkurnikov Aug 7, 2020

mkurnikov Aug 7, 2020

kszmigiel commented Aug 7, 2020


		base_manager_info = self.callee.expr.node

		if base_manager_info is None and not self.defer_till_next_iteration(reason='base_manager_info is None'):

Added dynamic class hook for from_queryset manager #427

Added dynamic class hook for from_queryset manager #427

Conversation

kszmigiel commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kszmigiel Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kszmigiel Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

kszmigiel commented Aug 5, 2020

sobolevn left a comment

Choose a reason for hiding this comment

sobolevn commented Aug 6, 2020

kszmigiel commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kszmigiel commented Aug 7, 2020

Added dynamic class hook for `from_queryset` manager #427

Added dynamic class hook for `from_queryset` manager #427

kszmigiel Jul 19, 2020 •

edited

Loading

kszmigiel Aug 5, 2020 •

edited

Loading