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

Support and examples for Class-Based Views (HTTPMethodView) #64

Merged
merged 5 commits into from
May 22, 2019
Merged

Support and examples for Class-Based Views (HTTPMethodView) #64

merged 5 commits into from
May 22, 2019

Conversation

fMeow
Copy link
Contributor

@fMeow fMeow commented May 10, 2018

Add support for Class-Based Views as issues #12 and #42 have pointed out.

In addition, a simple example is added to demonstrate that in class based view, sanic-openapi can actually work.

Examples for class-based views is also added to car examples.

@tudormunteanu
Copy link

Any idea when this will be merged in?

@anna-hope
Copy link

Is this adding something different than #67?

@fMeow
Copy link
Contributor Author

fMeow commented Jul 2, 2018

You can install forked version via pip and see what's different. @notnami

I think that the main difference are examples and tags support for blueprint.

And I didn't notice that there had been some PR trying to provide class-based views support when I wrote my own solution.

@fMeow
Copy link
Contributor Author

fMeow commented Jul 2, 2018

@tudormunteanu I think that sanic-openapi is no longer under maintenance. Besides, I need support for OpenAPI 3 for bearer token authentication. And that's why I turn to api-spec with some tricky code.
If anyone is interested, I can provide an example to show how to incorporate api-spec into sanic as it does not provide official support for sanic.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

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

How is this different than #67? Do we need this still? Can we get rid of one of them?

@logileifs
Copy link

Can we get this pull request or 67 merged?
Is there something I can do to help? I could resolve the conflicts if that's the only thing keeping this from being merged

@ahopkins
Copy link
Member

@chenjr0719 Can you take a look at these two and decide which one needs to be merged?

@logileifs The conflicts, but also understanding the overlap with #67.

@ahopkins ahopkins dismissed their stale review May 13, 2019 19:41

move forward

chenjr0719
chenjr0719 previously approved these changes May 14, 2019
Copy link
Member

@chenjr0719 chenjr0719 left a comment

Choose a reason for hiding this comment

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

Looks great and verified with Sanic 19.3.1, everything works. But I'm afraid this PR is very behind to current master. There is no openapi.py in current master, not sure what would happen if we merge it.

if hasattr(route.handler, 'view_class'):
# class based view
view = route.handler.view_class
for http_method in HTTP_METHODS:
Copy link
Member

Choose a reason for hiding this comment

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

BTW, here is a replacement if we don't want to import HTTP_METHODS:

for http_method in route.methods

@fMeow
Copy link
Contributor Author

fMeow commented May 16, 2019

I'm glad that this PR is receiving serious attention to bring class-based views to sanic-openapi. This PR has been around for a long time and may not match the current code of sanic-openapi.

So what can I help?

@chenjr0719
Copy link
Member

@Guoli-Lyu Glad to have you back. After version 0.5.0, the openapi.py and swagger.py have been merged into one file. You can find exactly the same code snippet which you want to modify at current swagger.py.
https://github.com/huge-success/sanic-openapi/blob/f82af54c06ec1cb56d1e8ae361fa1ecc9656fe07/sanic_openapi/swagger.py#L78-L84
https://github.com/huge-success/sanic-openapi/blob/f82af54c06ec1cb56d1e8ae361fa1ecc9656fe07/sanic_openapi/swagger.py#L105-L106

So, what you have to do are:

  1. Rebase your branch with current master.
  2. Porting the changes you made from openapi.py to swagger.py.

If there is any problem, just let me know.

@fMeow
Copy link
Contributor Author

fMeow commented May 16, 2019

Ok, I have done rebasing and porting. @chenjr0719

Copy link
Member

@chenjr0719 chenjr0719 left a comment

Choose a reason for hiding this comment

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

Great, appreciate your awesome PR. @ahopkins This PR is ready to merge.

@fMeow
Copy link
Contributor Author

fMeow commented May 22, 2019

What's the state of this PR? This PR have not merged yet.

@ahopkins ahopkins merged commit bb15e7e into sanic-org:master May 22, 2019
@ahopkins
Copy link
Member

@Guoli-Lyu Thanks for the nudge. I was waiting for the tests to complete and then forgot about it.

@junhoyeo
Copy link

junhoyeo commented May 27, 2019

Hope it update soon.

@ahopkins
Copy link
Member

@junhoyeo what do you mean? it is merged.

@chenjr0719
Copy link
Member

@ahopkins I think what @junhoyeo talking about is to publish a new release on PyPI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants