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

Generate schema with respect to http_method_names provided by CBV #5085

Merged
merged 3 commits into from May 3, 2017
Merged

Generate schema with respect to http_method_names provided by CBV #5085

merged 3 commits into from May 3, 2017

Conversation

ghost
Copy link

@ghost ghost commented Apr 19, 2017

Description

This limits the schema generation by http_method_names attribute (Django CBV). I just updated get_allowed_method to have a set intersection check.

Closes #4691.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 19, 2017

Hi @hurturk. Thanks for your contribution ! Do you think you could get a test case along with the PR ?

@ghost
Copy link
Author

ghost commented Apr 19, 2017

Hi @xordoquy, I already included a test case that overrides ExampleViewSet to remove permission classes and to specify http_method_names without POST. Though head and options don't have effect on schema, they are usually added for convention when attribute is used. Hope that helps.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 19, 2017

@hurturk woops, indeed. I'm getting some more coffee and I'm sorry for the previous comment.

@ghost
Copy link
Author

ghost commented Apr 19, 2017

Oh, I can also move change in PR around routers here after #4691 is decided. @xordoquy

return [method.upper() for method in callback.actions.keys()]
return [
method.upper() for method in
set(callback.actions.keys()).intersection(set(callback.cls().http_method_names))
Copy link
Member

@tomchristie tomchristie May 2, 2017

Choose a reason for hiding this comment

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

There's too much going on inline here.
I'd suggest:

  • Pull each of those two sets out into variable assignments.
  • Use the & operator, rather then .intersection(...)

Copy link
Member

@tomchristie tomchristie left a comment

Nice work, tho one requested change.

@ghost
Copy link
Author

ghost commented May 2, 2017

Hi @tomchristie, thank you for taking time on this. I have moved them to variables as you've pointed, hope that looks better.

@@ -246,7 +246,9 @@ def get_allowed_methods(self, callback):
Return a list of the valid HTTP methods for this endpoint.
"""
if hasattr(callback, 'actions'):
return [method.upper() for method in callback.actions.keys()]
actions = set(callback.actions.keys())
http_method_names = set(callback.cls().http_method_names)
Copy link
Member

@tomchristie tomchristie May 3, 2017

Choose a reason for hiding this comment

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

We shouldn't need to instantiate the class, right?

Copy link
Author

@ghost ghost May 3, 2017

Choose a reason for hiding this comment

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

Right, looks defensive. I will remove that quick.

@tomchristie tomchristie added this to the 3.6.3 Release milestone May 3, 2017
@tomchristie
Copy link
Member

tomchristie commented May 3, 2017

Seems good to me, yup.

Don't think I'd realised that http_method_names is public API, so... 👍

@tomchristie tomchristie merged commit 9731269 into encode:master May 3, 2017
1 check passed
@tomchristie
Copy link
Member

tomchristie commented May 3, 2017

Thanks for your time on this!

@ghost ghost deleted the schema-method-limited branch May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants