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
[FIX] OAS 3.0 yaml spec bundling error #779
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please also add the unit test cases for issue we're solving?
@@ -14,20 +14,20 @@ info: | |||
email: apiteam@wordnik.com | |||
license: | |||
name: Apache 2.0 | |||
url: 'http://www.apache.org/licenses/LICENSE-2.0.html' | |||
url: http://www.apache.org/licenses/LICENSE-2.0.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-v-singh Why are we making this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I locally ran npm test
and the test cases were failing, showing there is a difference between actual and expected output.
This could have possibly happened as we changed the js-yaml
version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define what actually changed? This much changes means there might be some functionality that changes and we should be very aware of such change. It could suddenly break the existing working definitions for users.
Let's pinpoint the exact issue and if there's way to avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was possibly introduced here here.
We can use quotingType
and forceQuotes
, but this will force the string style to be a specific way and we would need to change the expected files anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-v-singh So with new version, what would these URLs be treated as? Are they not string anymore? I get that we have to change expected files but what would happen with users that have such files, would they be able to generate the collection? would there collection generated be any different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are still strings
. Only the bundled files in yaml format would be produced without any string quotes, and yes they would be able to generate collections. The generated collections will not be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, in that case it's fine.
bundle.js
js-yaml
version to handle undefined values. Read More.