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

[FIX] OAS 3.0 yaml spec bundling error #779

Merged
merged 2 commits into from Jan 18, 2024

Conversation

aman-v-singh
Copy link
Collaborator

@aman-v-singh aman-v-singh commented Jan 16, 2024

  • Fixed logic in bundle.js
  • Bumped up js-yaml version to handle undefined values. Read More.
  • Fixed expected values for test cases.

@aman-v-singh aman-v-singh marked this pull request as ready for review January 16, 2024 05:34
Copy link
Member

@VShingala VShingala left a 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
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

package.json Outdated Show resolved Hide resolved
@VShingala VShingala merged commit 6e99c16 into develop Jan 18, 2024
6 checks passed
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.

None yet

2 participants