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

Add ability to disable OAS compacted emission #597

Closed
wants to merge 26 commits into from

Conversation

postatum
Copy link
Contributor

This PR addresses issues discussed here #364. In particular it makes possible to disable OAS 2.0, OAS 3.0 "compacted" emission added by this commit f4a8294.

Compacted emission can be disabled with RenderOptions().withoutCompactedEmission.

amf-core part of necessary changes is here aml-org/amf-core#98

I suggest you squish the PR as it contains a lot of unnecessary commits I did when trying to figure out how things work.

@postatum
Copy link
Contributor Author

The build fails because amf-core PR was not merged/released yet:

[error] /home/travis/build/aml-org/amf/amf-webapi/shared/src/main/scala/amf/plugins/document/webapi/OasPlugin.scala:172:17: value isWithCompactedEmission is not a member of amf.core.emitter.RenderOptions

[error]     if (options.isWithCompactedEmission) {

@jstoiko jstoiko requested a review from gutee September 22, 2020 22:39
@AgustinBettati
Copy link
Contributor

Hello @postatum, thank you for opening the PR.
I have a few comments to add regarding the changes you have made. You will see that OasSpecEmitterContext has a Boolean variable to disable compactEmission. Using this variable directly will significantly reduce the amount of code that has to be changed, there is no need to create the InlinedContext which forces you to to handle specific cases in emission. You will only have to modify RenderOptions, both OasPlugins in the specContext method, and include tests.
It would be great if your could update both of your branches (in amf and amf-core) with these adjustments and squash them into a single commit.

@postatum
Copy link
Contributor Author

postatum commented Sep 29, 2020

Hello @postatum, thank you for opening the PR.
I have a few comments to add regarding the changes you have made. You will see that OasSpecEmitterContext has a Boolean variable to disable compactEmission. Using this variable directly will significantly reduce the amount of code that has to be changed, there is no need to create the InlinedContext which forces you to to handle specific cases in emission. You will only have to modify RenderOptions, both OasPlugins in the specContext method, and include tests.
It would be great if your could update both of your branches (in amf and amf-core) with these adjustments and squash them into a single commit.

Hi @AgustinBettati!
Thanks for the feedback. I reworked the PR the way you've suggested and created a different PR with squished commits: #668

@postatum postatum closed this Sep 29, 2020
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.

2 participants