typeddjango / djangorestframework-stubs

PEP-484 stubs for django-rest-framework
MIT License
439 stars 115 forks source link

Allow `http.HTTPMethod` enum values in `@action()` decorator #512

Closed sshishov closed 10 months ago

sshishov commented 10 months ago

I have made things!

Added possibility to specify http.HTTPMethod enum as an action's method

Related issues

Fixes: https://github.com/typeddjango/djangorestframework-stubs/issues/396

Notes for reviewer:

Please feel free to comment and I will make appropriate changes. @intgr 🎉

intgr commented 10 months ago

I have deleted the Sequence for lowercase as an options for action specification, apparently there is no test for it.

We don't usually add regression tests for the more straightforward parts of type stubs. Only particularly complicated generics, decorators or @overloads are worth tests. In most other cases, tests would create too much maintenance overhead.

sshishov commented 10 months ago

What I have noticed, these tests are quite slow, every test is around 1 second (roughly) and therefore having a lot of tests to cover all cases (for regressing) is not quite applicable there. But regression can help in identifying something broken, especially with different versions of the parent package as sometimes things changes over time in the core package. But yeah, it does not make sense to add it here, it is not a critical software which will break everything, is something will be broken, we can just fix it and release in new version.

Regarding this point:

I have deleted the Sequence for lowercase as an options for action specification, apparently there is no test for it. I even do not know if it is correct, will double check later if UPPERCASE and HTTPMethod can be specified there. I just do not know why they are different.

According to this: https://github.com/encode/django-rest-framework/blob/f378f98a401f28df4ef9bdaf3ee56a1017c195ab/rest_framework/decorators.py#L123C1-L123C1, the methods can be mixed case, therefore we are good with changes, I guess.

intgr commented 10 months ago

According to this: https://github.com/encode/django-rest-framework/blob/f378f98a401f28df4ef9bdaf3ee56a1017c195ab/rest_framework/decorators.py#L123C1-L123C1, the methods can be mixed case, therefore we are good with changes, I guess.

EDIT: Sorry, I noticed now I forgot to post the review that I had finished.

You're getting two different things mixed up. Yes, the action(methods=) parameter accepts mixed case as input.

_LOWER_CASE_HTTP_VERBS was only used in the ViewSetAction protocol, which is the return value from @action decorator. Apparently in previous versions, the decorator used to add a methods attribute to the decorated function, that had lowercased methods.

But that's all moot anyway, like I said , the method attribute no longer exists on @action decorated functions in latest DRF version.

intgr commented 10 months ago

As a follow-up, should we use _HttpMethod type in the api_view() decorator too?

sshishov commented 8 months ago

Hi @intgr to be honest I do not know, but yeah, we are using it in PROD and it is working as expected