search

typeddjango / djangorestframework-stubs

PEP-484 stubs for django-rest-framework
MIT License
453 stars 120 forks source link

has_permission more appropriate view arg #104

Open amikrop opened 4 years ago

amikrop commented 4 years ago

Lines 10 and 11 of https://github.com/typeddjango/djangorestframework-stubs/blob/master/rest_framework-stubs/permissions.pyi declare the view argument as APIView while it can be a ViewSet (or similar) too (which, of course, inherits from APIView). Annotating it in my code as ModelViewSet (or MyCustomViewSet) causes this error though https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides If I try to annotate the view argument as an APIView in my code, I get problems because I use view.action inside of my has_permission, which doesn't exist for APIView. Not sure this is a required fix in djangorestframework-stubs or in my code.

kalekseev commented 4 years ago

This was changed in the https://github.com/typeddjango/djangorestframework-stubs/commit/cf9d492ec01f99e9a3fa89558aa1fbf1a749bc22#diff-19f9224eec09114fbeb9cb2aa07f53c2f1efb422a42812ec6a9bebe74e8993caL10, there are many things that changed in this pr to use more specific types, I've created https://github.com/typeddjango/djangorestframework-stubs/pull/101 where I rollback some of that changes, I can include change to permission too in that pr, but I won't have time until next week to finish it (I need to write some test in order to merge it). cc @Goldziher

kalekseev commented 4 years ago

Oh, I misunderstood your problem on first read, just annotate it as APIView and use if isinstance(view, ViewSet) then you will be able to access view.action

amikrop commented 4 years ago

Alright, thank you.

MarcinWieczorek commented 3 years ago

@kalekseev because you did good work with last PR please find time to fix this issue. I'm willing to help, because I think your first comment here is valid.

kalekseev commented 3 years ago

@kalekseev because you did good work with last PR please find time to fix this issue. I'm willing to help, because I think your first comment here is valid.

I think my second comment addressed that situation and we don't need to change this, at least I don't have evidence that we should replace APIView with View in these methods.