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

Improve enum values for Enum Type in Swagger ReadOnly documentation #4191

Merged
merged 32 commits into from Apr 6, 2018

Conversation

dedece35
Copy link
Contributor

@dedece35 dedece35 commented Feb 4, 2018

SwaggerUI documentation in ReadOnly mode gives :

One enum items representation wasn't considered in swagger.json
Currently considered in swagger.json :
... "parameters": [ { "name": "status", "in": "query", "description": "Status values that need to be considered for filter", "required": true, "type": "array", "items": { "type": "string", "enum": [ "available", "pending", "sold" ], "default": "available", "example": "pending" }, "collectionFormat": "multi" } ], ...

Not considered in swagger.json (example swagger.json generated from swagger annotations in Java code) - there is no "items" structure for "enum" values :
... "parameters": [ { "name": "stockstatus", "in": "query", "description": "stock status values that need to be considered for filter", "required": false, "type": "string", "default": "full", "x-example": "empty", "enum": [ "empty", "halfempty", "full" ] } ], ...

Description

  • rename variables for enum managment
  • add managment for new structure ("enum", "x-example", "default")
  • add example json swagger ("swagger-petstore-enum.json") for developers (we can now add "http://localhost:3200/swagger-petstore-enum.json" to test new path added for this feature ("/store/findByStock") )

Motivation and Context

As I described in my old PR #3977 , I think it's good idea to have access to default value, example value et availables values in readonly mode (not in TryItOut mode).
This PR is able to solve a new situation in swagger.json (enum values can be out of "items" structure)

There is NO issue for this feature

Fixes #2873.

How Has This Been Tested?

I tested in the same way as my old PR #3977

"npm test" results :
236 passing (744ms) 11 pending

Screenshots (if appropriate):

the red square zone is the result with new path added ("/store/findByStock") and new Enum values managment
swagger-ui-bis

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@dedece35
Copy link
Contributor Author

do you have some news about this pull request ?

@dedece35
Copy link
Contributor Author

@shockey ? :)

@webron
Copy link
Contributor

webron commented Feb 20, 2018

@dedece35 we've slated time next week to address open PRs, check their functionality, give feedback, and merge if appropriate. Thanks for submitting the PR, and thanks for waiting. We appreciate the community contributions, just a bit overwhelmed with work :)

@dedece35
Copy link
Contributor Author

@webron OK ... thanks for your answer :)

@dedece35
Copy link
Contributor Author

@webron I let myself to revive you about my pull-request :)
I'm really expecting a lot of this PR to integrate it in dev in many REST APIs in my french society.

thanks again :)

@webron
Copy link
Contributor

webron commented Mar 19, 2018

@dedece35 - I'm willing to go ahead and merge it (pending code review) for the functionality - but keep in mind that we may change it completely in the future. The need is understandable, which is why I think we should move forward with it, but design wise it may not be the best solution (no offense).

@@ -0,0 +1,1079 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason this file was added?

Copy link
Contributor Author

@dedece35 dedece35 Mar 23, 2018

Choose a reason for hiding this comment

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

hello @shockey, this file is a simple copy of original http://petstore.swagger.io/v2/swagger.json with a new use case for enum values. In original swagger-petstore.json you can find enum values inside "get/parameters/items" ressource (example with path "/pet/findByStatus"). In the new swagger JSON file, "enum" values can appear in "get/parameters" and not only in "items" ressource (example with path "/pet/findByStock"). Indeed, this file is for testing to check enum values are also processed if are'nt inside "items" ressource : developer can use http://localhost:3200/swagger-petstore-enum.json to see the swagger-ui rendering


if (isDisplayParamItemsEnum) { // if we have an array, default value is in "items"
paramDefaultValue = paramItems.get("default")
if (paramExample == undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use === instead of ==

Copy link
Contributor Author

@dedece35 dedece35 Mar 23, 2018

Choose a reason for hiding this comment

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

ok, no problem.

isDisplayParamItemsEnum = true

if ( paramEnum !== undefined ) {
if (paramEnum.size > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These two conditionals should be collapsed into one if statement

Copy link
Contributor Author

@dedece35 dedece35 Mar 23, 2018

Choose a reason for hiding this comment

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

ok, no problem.

if ( paramItems !== undefined ) {
paramItemsEnum = param.get("items").get("enum")

if (paramItems !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These statements should be inverted: the if statement should have a positive condition (paramItems === undefined) and the else statement should be used for the negative case.

Copy link
Contributor Author

@dedece35 dedece35 Mar 23, 2018

Choose a reason for hiding this comment

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

in this condition, I have no positive case : I only want to do things in negative case. I can't translate this condition in positive way.

Copy link
Contributor Author

@dedece35 dedece35 left a comment

Choose a reason for hiding this comment

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

@shockey changes done.

@shockey shockey merged commit 39d3452 into swagger-api:master Apr 6, 2018
@shockey
Copy link
Contributor

shockey commented Apr 6, 2018

thanks @dedece35! 😄

@dedece35
Copy link
Contributor Author

dedece35 commented Apr 7, 2018

thanks to you :)

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.

Allowable values only displayed after pressing "Try it out"
3 participants