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

Including empty object causes errors in doc validation #70

Closed
wants to merge 1 commit into from

Conversation

bbrizzi
Copy link

@bbrizzi bbrizzi commented Jun 23, 2020

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Test
  • Docs
  • Refactor
  • Build-related changes
  • Other, please describe:

Description:

Hey, thanks for developping this library, it's great !

I use only the processSwagger() part so I can generate and host the file separately. This makes debugging the swagger.json file easier for me.

I noticed that when I don't add contact and license to my options object, the library stills adds them with a default value of empty object which causes an error in swaggger's validator :

Structural error at info.license should have required property 'name' missingProperty: name

The fix in this PR replaces the default {} value with undefined to fix this error. I didn't run any tests but it doesn't look like it breaks anything.

Thank you !

contact: options.contact ? getContact(options.contact) : {},
license: options.license ? license(options.license) : {},
contact: options.contact ? getContact(options.contact) : undefined,
license: options.license ? license(options.license) : undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. We would like to merge this PR 👍. However, is it possible to change this to instead add undefined as default value just remove the key of the final object?

...(options.contact ? { contact: getContact(options.contact) } : {}),
...(options.license ? { license: license(options.license) } : {}),

@@ -9,7 +9,7 @@ const setServer = (server = {}) => ({
type: 'string',
defaultValue: '',
}),
variables: server.variables ? server.variables : {},
variables: server.variables ? server.variables : undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here.

...(server.variables ? { variables: server.variables } : {}),

Copy link
Member

@kevinccbsg kevinccbsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add those changes I think we can merge this. There are some failing tests, if you want you could also fix them. If you don't want to we can have a look after we merge this, and we will be able to do an npm release.

We were also wondering what swagger validator tool are you using? so that we can have a look and prevent this kind of error in the following versions 👍 We used this one.

@kevinccbsg
Copy link
Member

Hi, @bbrizzi could you have time to update this PR? Are you still interested in adding this to this package? 😃

@kevinccbsg
Copy link
Member

We are going to cover this case in this issue #73 thanks for the feedback. We really appreciate it.

@kevinccbsg kevinccbsg closed this Jul 9, 2020
@kevinccbsg
Copy link
Member

Hi, @bbrizzi this release contains the changes you requested in this PR.

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