Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 10 commits into from
Nov 19, 2023

Conversation

sshishov
Copy link
Contributor

@sshishov sshishov commented Nov 18, 2023

I have made things!

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

Related issues

Fixes: #396

Notes for reviewer:

  • as this feature is added only on Python 3.11 I tried to do special treatment (check againt sys.version_Info, just do not know if I did it right, especially in the tests. It would actually more proper to mark test as Python3.11+ only and add in that test only the line with HTTPMethod and on previous versions this test will be just skipped. But I do not know how to do it.
  • 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.
  • added 2 extra tests, one for UPPERCASE as it was not tested and one for HTTPMethod

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

Signed-off-by: Sergei Shishov <sshishov.sshishov@gmail.com>
@sshishov sshishov force-pushed the ss/add-httpemethod-support branch from 4957d96 to a25faea Compare November 18, 2023 11:28
Signed-off-by: Sergei Shishov <sshishov.sshishov@gmail.com>
Signed-off-by: Sergei Shishov <sshishov.sshishov@gmail.com>
Signed-off-by: Sergei Shishov <sshishov.sshishov@gmail.com>
Signed-off-by: Sergei Shishov <sshishov.sshishov@gmail.com>
@intgr
Copy link
Contributor

intgr commented Nov 19, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

intgr commented Nov 19, 2023

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.

sshishov and others added 4 commits November 19, 2023 22:45
…tAction`

Signed-off-by: Sergei Shishov <sshishov.sshishov@gmail.com>
Signed-off-by: Sergei Shishov <sshishov.sshishov@gmail.com>
NOTE: the `Sequence` used now in place instead of adding to type alias

Signed-off-by: Sergei Shishov <sshishov.sshishov@gmail.com>
Copy link
Contributor

@intgr intgr left a comment

Choose a reason for hiding this comment

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

Thanks, this is great. I just pushed a commit to add back a few empty lines :)

@intgr intgr changed the title fix(action): add support of http.HTTPMethod to action decorator Allow using http.HTTPMethod enum values in @action() decorator Nov 19, 2023
@intgr intgr changed the title Allow using http.HTTPMethod enum values in @action() decorator Allow http.HTTPMethod enum values in @action() decorator Nov 19, 2023
@intgr intgr merged commit 6af878b into typeddjango:master Nov 19, 2023
@intgr
Copy link
Contributor

intgr commented Nov 19, 2023

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

@sshishov
Copy link
Contributor Author

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.11, support new http.HTTPMethod enum in action and view
2 participants