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

OAS3 support for sanic-openapi3 #210

Merged
merged 10 commits into from
Mar 16, 2021
Merged

Conversation

artcg
Copy link
Contributor

@artcg artcg commented Mar 16, 2021

See historic conversation in

#194

and

#179

@artcg
Copy link
Contributor Author

artcg commented Mar 16, 2021

Checking the merge conflict...

@artcg
Copy link
Contributor Author

artcg commented Mar 16, 2021

n.b. on the tests @astagi has some ideas for improvements we discussed via email, but could be good for a separate pull requests to get this out of the way

@ahopkins
Copy link
Member

Looks great. Thanks to @artcg @ioggstream @ruckc and anyone else that helped in this.

What about documentation?

@ahopkins
Copy link
Member

n.b. on the tests @astagi has some ideas for improvements we discussed via email, but could be good for a separate pull requests to get this out of the way

Yes. @astagi, seperate PR.

@ahopkins ahopkins merged commit 437f1d6 into sanic-org:master Mar 16, 2021
@artcg
Copy link
Contributor Author

artcg commented Mar 16, 2021

Documentation can mostly be adapted from the current doc except replacing swagger_blueprint for oas3_blueprint and replacing the decorators as necc.

Maybe someone who feels like it could look at it with some examples when they have some free time

@astagi
Copy link
Contributor

astagi commented Mar 16, 2021

n.b. on the tests @astagi has some ideas for improvements we discussed via email, but could be good for a separate pull requests to get this out of the way

Yes. @astagi, seperate PR.

Ok, I'll improve tests in another PR. Tests for oas3 implementation use @doc decorator from oas2, however oas3 can't recognize it, it has a different implementation. The reason why tests are not failing is that the JSON returned by app and benchmark app is the same, but it's not correct.

@ahopkins ahopkins mentioned this pull request Mar 16, 2021
1 task
@artcg artcg mentioned this pull request Mar 17, 2021
@artcg artcg deleted the ruck3-bugfix branch March 17, 2021 08:20
@astagi
Copy link
Contributor

astagi commented Mar 17, 2021

@artcg @ahopkins before opening an issue or start working on the feature, I'd like to discuss a possible code refactoring. oas3 package has its own blueprint and its own doc module, I think that in order to avoid confusion for the final user we should force developers to import blueprint and doc modules from oas3 module, for example

from sanic_openapi.oas3 import swagger_blueprint, doc
app.blueprint(swagger_blueprint)

what do you think?

@artcg
Copy link
Contributor Author

artcg commented Mar 17, 2021

Sure that is a nice idea, I think also necessary to keep the other import for backcompatibility, so

from sanic_openapi import doc, swagger_blueprint —> v2

from sanic_openapi.swagger2 import doc, swagger_blueprint —> v2

from sanic_openapi.oas3 import openapi/doc, oas3_blueprint —> v3

sent from my phone sorry for poor formatting

@ahopkins
Copy link
Member

I'm not personally keen on the naming difference: swagger2 v oas3. And, as much as possible it is helpful to keep the most used imports at the top level (v21.3 makes some helpers like sanic.response.text and sanic.response.json available at the root).

Now, I get it that this is somewhat different, and as I am not a user of this package I really know little about its architecture. I just push the merge button when someone else needs me to 😆

On a different note, there was some conversation that the @sanic-org/steering-council had about this package in general at our last meeting. That you guys are actively working on it is great news. I know that @vltr had some ideas on ways to improve or to take this going forwards.

@artcg
Copy link
Contributor Author

artcg commented Mar 17, 2021

Yes I agree about that, the main reason for the differences there is that when openapi3 released they changed the naming usage in some cases, but could be a good idea to just make it the same across the board anyway,

personally I like openapi better, but swagger is needed for the json file output to keep intact with the spec for the ui I believe

@artcg
Copy link
Contributor Author

artcg commented Mar 17, 2021

Also I think breaking backcompatibility with the import naming wouldn't be the end of the world, since if anyone is using this in production they should be specifying the version anyway, and then it is an easy fix on their end to change the naming

@artcg
Copy link
Contributor Author

artcg commented Mar 17, 2021

So I think to conclude, something like this is my preference, definitely happy to hear other peoples thoughts too:

/sanic_openapi/swagger2 -> /sanic_openapi/openapi2

/sanic_openapi/oas3 -> /sanic_openapi/openapi3

sanic_openapi/__init__.py

from .openapi2 import openapi2_blueprint
from .openapi3 import openapi3_blueprint
from . import doc  # for openapi2 decorators and object classes which are compatible with both versions
from .openapi3 import openapi  # for openapi3 decorators


swagger_blueprint = openapi2_blueprint  # backcompatibility
sanic_openapi/openapi2/__init__.py

from .blueprint import blueprint_factory

openapi2_blueprint = blueprint_factory()
sanic_openapi/openapi3/__init__.py

from .blueprint import blueprint_factory
# maybe here, from .. import doc  - to provide same level access to doc.String etc ? 

openapi3_blueprint = blueprint_factory()

sanic_openapi/swagger.py -- removed

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.

4 participants