swagger_ui: add directives that provide API Explorer and spec information #79

ergo · 2018-03-25T19:05:49Z

easy way to register and serve API explorer for Swagger/OpenAPI schemas

coveralls · 2018-03-25T19:08:55Z

Coverage remained the same at 100.0% when pulling 8d647a0 on ergo:master into a5182fe on Cornices:master.

coveralls · 2018-03-25T19:08:55Z

Coverage remained the same at 100.0% when pulling 8d647a0 on ergo:master into a5182fe on Cornices:master.

coveralls · 2018-03-25T19:08:55Z

Coverage remained the same at 100.0% when pulling 8d647a0 on ergo:master into a5182fe on Cornices:master.

coveralls · 2018-03-25T19:08:55Z

Coverage remained the same at 100.0% when pulling 8d647a0 on ergo:master into a5182fe on Cornices:master.

coveralls · 2018-03-25T19:08:55Z

Coverage remained the same at 100.0% when pulling 41e4886 on ergo:master into a5182fe on Cornices:master.

leplatrem

Thanks for your contribution!

Will the old method still work? I guess so right? If I don't call the new config directive, I can still do it the old way...

I think it's cool to have this, as long as it remains optional, but I'd rather let Gabi give a final review :)

leplatrem · 2018-03-26T15:10:44Z

cornice_swagger/templates/index.html

+<head>
+  <meta charset="UTF-8">
+  <title>Swagger UI</title>
+  <link href="https://fonts.googleapis.com/css?family=Open+Sans:400,700|Source+Code+Pro:300,600|Titillium+Web:400,600,700" rel="stylesheet">


nitpick: Ship the font here maybe instead of leaking to google?

The old method will still work.

We would need to ship statics and update them in the package - the fonts link exists also in the official Swagger UI package, its not something I've made - I did point to swagger UI JS files on CDN to avoid shipping big statics.

leplatrem · 2018-03-26T15:13:27Z

docs/source/quickstart.rst

+
+Then you will be able to access Swagger UI API explorer on url:
+
+http://localhost:8000/api-explorer (in case of example below)


Yup, I've just added some defaults for this to work, no functionality of existing package is removed or altered.

I think what @leplatrem said is the the example is literally above (not below). Also you can only use "in the example above" (remove "case of").

Ah, got confused because I was thinking about the "minimalist example below" :)

leplatrem · 2018-03-26T15:14:47Z

docs/source/tutorial.rst



 If you don't know what this is about or need more information, please check the
 `Pyramid documentation <http://docs.pylonsproject.org/projects/pyramid>`_

+By default API explorer will be served under `/api-explorer` path in your
+application. You can easily configure the paths, required pyramid view
+permissions or route factory.


I think it feels more natural to read configure the paths, required permissions and Pyramid route factory

Thanks, not a native speaker will correct that.

I think it's worth mention that additional kwargs should match the the generate() arguments.

Thanks! Will fix that.

leplatrem · 2018-03-26T15:15:05Z

docs/source/tutorial.rst

+Custom Swagger UI <script> bootstrap
+====================================
+
+By default standard Swagger UI config is used, but you can customize the


link to Swagger UI project?

leplatrem · 2018-03-26T15:15:26Z

docs/source/tutorial.rst

+
+The default one is:
+
+`cornice_swagger.swagger_ui_script_generator = cornice_swagger.views:swagger_ui_script_template`


In RST you have to double back quotes

leplatrem · 2018-03-26T15:16:01Z

docs/source/tutorial.rst

+
+`cornice_swagger.swagger_ui_script_generator = cornice_swagger.views:swagger_ui_script_template`
+
+It points to following callable that accepts request object:


to the following

a request object

leplatrem · 2018-03-26T15:20:38Z

cornice_swagger/__init__.py

+                    route_name='cornice_swagger.swagger_ui_path')
+    config.add_view('cornice_swagger.views.swagger_json_view',
+                    renderer='json', permission=permission,
+                    route_name='cornice_swagger.swagger_api_path')


My feedback would be :

I like the idea of using a config directive

I would find a better name, not using the deprecated swagger name, and not mentioning UI (enable_openapi() ?)

I would set swagger_ui_path to None by default

I would would not register the UI views if the swagger_ui_path is None

Sure, makes sense, let me correct that.

leplatrem · 2018-03-26T15:21:10Z

cornice_swagger/views.py

+    ui_js_bundle_url = 'https://cdnjs.cloudflare.com/ajax/libs/' \
+                       'swagger-ui/3.12.9/swagger-ui-bundle.js'
+    ui_js_standalone_url = 'https://cdnjs.cloudflare.com/ajax/libs/' \
+                           'swagger-ui/3.12.9/swagger-ui-standalone-preset.js'


nitpick: move as module constants?

leplatrem · 2018-03-26T15:25:25Z

cornice_swagger/views.py

+    doc = cornice_swagger.CorniceSwagger(cornice.service.get_services())
+    kwargs = request.registry.settings['cornice_swagger.spec_kwargs']
+    my_spec = doc.generate(**kwargs)
+    return my_spec


I think there should be an example on how to replace this with a custom view.

For example, in Kinto we inherit the CorniceSwagger class and instantiate our own stuff the the view :)

https://github.com/Kinto/kinto/blob/8db533939f47d33c44deb66e0d1285258d99d68b/kinto/core/views/openapi.py#L22-L32

I think if you want to replace this with a custom view - then you don't use the directive, because it basicly just adds 2 views so you don't have to override them. If someone wants to override they'd have to redo all the work directive does basicly.

gabisurita

Looking good! I like the idea of using a directive instead of manually adding a service to serve the documentation. I've got a few minor suggestions...

gabisurita · 2018-03-27T12:58:21Z

docs/source/tutorial.rst



 If you don't know what this is about or need more information, please check the
 `Pyramid documentation <http://docs.pylonsproject.org/projects/pyramid>`_

+By default API explorer will be served under `/api-explorer` path in your
+application. You can easily configure the paths, required pyramid view
+permissions or route factory.


I think it's worth mention that additional kwargs should match the the generate() arguments.

gabisurita · 2018-03-27T12:58:53Z

examples/minimalist.py

-def openAPI_spec(request):
-    doc = CorniceSwagger(get_services())
-    my_spec = doc.generate('MyAPI', '1.0.0')
-    return my_spec


Maybe move this to another example?

I think the minimalist example should show the easiest way to archieve that?
How to generate the docs is mentioned above so I'm not sure if it makes sense to duplicate the example just show that.

gabisurita · 2018-03-27T13:00:55Z

cornice_swagger/__init__.py

+    config.add_directive('register_swagger_ui', register_swagger_ui)
+
+
+def register_swagger_ui(


maybe registering theapi_path could be a separate directive, thus allowing one to register the api docs without using the api_explorer. What do you think?

Sure, sounds like a sane change.

@gabisurita any suggestions of what we want to name the other directive?

ergo · 2018-03-27T20:13:07Z

Hi @gabisurita, @leplatrem I've tried to do everything according to your feedback. We have now two directives, one just renders simple spec json one renders explorer.

leplatrem · 2018-03-27T21:17:37Z

Thanks it looks great :)

I have a little doubt left about naming being a little bit too vague. I feel that I'd prefer cornice_enable_openapi_view() and cornice_enable_openapi_explorer().
What do you think? Otherwise I'm +1 on landing this :)

ergo · 2018-03-27T21:18:36Z

Sure, let me correct that.

ergo · 2018-03-27T21:32:04Z

@leplatrem Done, the PR now has updated function names.

gabisurita · 2018-03-27T22:01:18Z

@ergo Just one nipick in the docs and we can GTM.

ergo · 2018-03-27T22:31:54Z

@gabisurita I've fixed the docs.

gabisurita · 2018-03-28T15:07:21Z

@ergo Thanks! Just merged and released 0.6.0.

swagger_ui: add a register_swagger_ui directive that provides

8d647a0

easy way to register and serve API explorer for Swagger/OpenAPI schemas

ergo mentioned this pull request Mar 25, 2018

Document Swagger UI integration #18

Closed

contributors: sign contributors file

1763e85

leplatrem reviewed Mar 26, 2018

View reviewed changes

gabisurita approved these changes Mar 27, 2018

View reviewed changes

swagger_ui: update the code with PR feedback

f97fef9

ergo changed the title ~~swagger_ui: add a register_swagger_ui directive that provides~~ swagger_ui: add directives that provide API Explorer and spec information Mar 27, 2018

swagger_ui: update function names

41e4886

leplatrem approved these changes Mar 27, 2018

View reviewed changes

gabisurita merged commit 5b2e383 into Cornices:master Mar 28, 2018

lnewson mentioned this pull request Apr 16, 2018

API Explorer templates not included in PyPI package #87

Closed


		Then you will be able to access Swagger UI API explorer on url:

		http://localhost:8000/api-explorer (in case of example below)


		The default one is:

		`cornice_swagger.swagger_ui_script_generator = cornice_swagger.views:swagger_ui_script_template`


		`cornice_swagger.swagger_ui_script_generator = cornice_swagger.views:swagger_ui_script_template`

		It points to following callable that accepts request object:

		config.add_directive('register_swagger_ui', register_swagger_ui)


		def register_swagger_ui(

swagger_ui: add directives that provide API Explorer and spec information #79

swagger_ui: add directives that provide API Explorer and spec information #79

Conversation

ergo commented Mar 25, 2018

coveralls commented Mar 25, 2018

coveralls commented Mar 25, 2018

coveralls commented Mar 25, 2018

coveralls commented Mar 25, 2018

coveralls commented Mar 25, 2018 • edited Loading

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabisurita Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabisurita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ergo commented Mar 27, 2018

leplatrem commented Mar 27, 2018

ergo commented Mar 27, 2018

ergo commented Mar 27, 2018

gabisurita commented Mar 27, 2018 • edited Loading

ergo commented Mar 27, 2018

gabisurita commented Mar 28, 2018

coveralls commented Mar 25, 2018 •

edited

Loading

gabisurita Mar 27, 2018 •

edited

Loading

gabisurita commented Mar 27, 2018 •

edited

Loading