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

feature: Support passing custom OpenAPI versions. #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

playpauseandstop
Copy link

@playpauseandstop playpauseandstop commented May 12, 2019

Previously AiohttpApiSpec instantiate ApiSpec only for openapi_version == '2.0', this PR changes it by allowing pass openapi_version to setup_aiohttp_apispec function and then use given value, while instantiating ApiSpec instance.

Fixes #46

Previously `AiohttpApiSpec` instantiate `ApiSpec` only for `openapi_version == '2.0'`,
this commit changes it by allowing pass `openapi_version` to `setup_aiohttp_apispec`
function and then use given value, while instantiating `ApiSpec` instance.

Fixes maximdanilchenko#46
@codecov-io
Copy link

codecov-io commented May 12, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@9119931). Click here to learn what that means.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #52   +/-   ##
=========================================
  Coverage          ?   94.35%           
=========================================
  Files             ?        5           
  Lines             ?      177           
  Branches          ?        0           
=========================================
  Hits              ?      167           
  Misses            ?       10           
  Partials          ?        0           
Impacted Files Coverage Δ
aiohttp_apispec/aiohttp_apispec.py 88.76% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9119931...f774f04. Read the comment docs.

@playpauseandstop
Copy link
Author

However, I failed to test documentation generator against OpenAPI 3.0, I tried to provide static schemas as done for 2.0, but for some reason it didn't work well for me.

Maybe cause OpenAPI 3.0 uses components.schemas instead of definitions and they should be populated other way.

@maximdanilchenko
Copy link
Owner

Hi! Thank you for your PR!

There already exists an issue #46 about it. As you mentioned, it is not only version changing, but there are some differences in Openapi schemas for 2.X and 3.X which we need to support (like in your example about definitions and maybe some other we generates before using apispec lib).

If you have enough time and aspiration - you could finish your PR (and it would be great). Anyway I have plans to support this feature in the near future.

@playpauseandstop
Copy link
Author

playpauseandstop commented May 13, 2019

@maximdanilchenko

Yeah, I mentioned #46 in PR description.

As of tests for OpenAPI 3.0 and its better integration, I will work on it, as interested in having OpenAPI 3.0 support in aiohttp-apispec :)

@agronholm
Copy link
Contributor

@playpauseandstop Do you plan on finishing this PR? If not, I might give it a shot myself.

@playpauseandstop
Copy link
Author

@agronholm

Unfortunately not.

I gave up here as decided to bring OpenAPI 3 support to aiohttp apps as done in pyramid_openapi3 by providing whole OpenAPI 3 schema and not to pregenerate it with apispec. This resulted in rororo library.

But, if you manage to finish this PR (or add OpenAPI 3 support to aiohttp-apispec via another PR) I'll be very happy.

@agronholm
Copy link
Contributor

The vast majority of these changes are just cosmetic code style changes.

@agronholm
Copy link
Contributor

@maximdanilchenko how far are you willing to go with the v3 conversion? Could we switch to v3 by default (would probably warrant a major version upgrade)?

@maximdanilchenko
Copy link
Owner

@agronholm, I'm agree. We can do this in next major release. But I want to leave it possible to choose between v2 and v3.

@agronholm
Copy link
Contributor

But I want to leave it possible to choose between v2 and v3.

I can do that, but are you on board with making v3 the default?

@maximdanilchenko
Copy link
Owner

But I want to leave it possible to choose between v2 and v3.

I can do that, but are you on board with making v3 the default?

Yes, sure. We can make it optional in some 2.x version, and after some stabilizations make it default in 3.0.

@ghost
Copy link

ghost commented Nov 3, 2019

Hi there, I'll be needing this change in the near future. Is there a timeline for this being merged shortly?

@agronholm
Copy link
Contributor

I've been looking into it but not even upstream apispec seems to have all the necessary bits in place. So don't hold your breath.

@ghost
Copy link

ghost commented Nov 3, 2019

Weird, I swear I've used it before. What's missing/needed?

@agronholm
Copy link
Contributor

I can't recall everything right now, but the things I can think of right now are security schemes, OneOf/AnyOf and others. I will try to post a more complete listing next time I'm looking at the code.

@ghost
Copy link

ghost commented Nov 3, 2019

The way I see it, its simply just passing an option to the underlying APISpec class. The marshmallow/apispec repo handles V3.

From my understanding of the code (correct me if I'm wrong) this change makes no breaking changes to this project and so can be safely merged and released?

@mickours
Copy link

We are eager to see this merged! Do you think it would be possible soon?

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.

support openapi 3.0
5 participants