-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
[scala] fix akka-scala-client template compilation warnings #5106
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
Changes from all commits
6bfd5b2
dbaeeba
7a4d981
ef3ac57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,15 @@ paths: | |
| description: '' | ||
| operationId: addPet | ||
| responses: | ||
| '200': | ||
| description: successful operation | ||
| content: | ||
| application/xml: | ||
| schema: | ||
| $ref: '#/components/schemas/Pet' | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Pet' | ||
| '405': | ||
| description: Invalid input | ||
| security: | ||
|
|
@@ -41,6 +50,15 @@ paths: | |
| description: '' | ||
| operationId: updatePet | ||
| responses: | ||
| '200': | ||
| description: successful operation | ||
| content: | ||
| application/xml: | ||
| schema: | ||
| $ref: '#/components/schemas/Pet' | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Pet' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chameleon82 are these a must? In other words, would the change still work without documenting the HTTP 200 response in the spec? We prefer not to update petstore.yaml to keep it as original as possible.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wing328 Thanks for calling this out. When you say original, what are we keeping it original compared to? This spec isn't the "official" petstore.yaml spec from OpenAPI Specification 3.0 examples. I'd have some concerns about not including these changes. It would mean that the PUT and POST methods are only documented to ever return a client error. From https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#responsesObject:
Although the line "documentation is expected to cover a successful operation response" doesn't use the specification terminology
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wing328 @jimschubert I've checked than one https://petstore3.swagger.io/api/v3/openapi.yaml which currently has those specified, openApi specs also have specified successful response here https://github.com/OAI/OpenAPI-Specification/blob/master/examples/v3.0/petstore.yaml and here https://github.com/OAI/OpenAPI-Specification/blob/master/examples/v3.0/petstore-expanded.yaml
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that's probably why they've used @wing328 are you cool with keeping these changes? We should probably also review all our go-to specs and make sure they represent real-world designs.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agreed with reviewing the spec to ensure it's correct. Let's go with this PR for the time being. |
||
| '400': | ||
| description: Invalid ID supplied | ||
| '404': | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| sbt.version=1.2.8 | ||
| sbt.version=1.3.6 |
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 will be a potential breaking change unless this is added to the constructor:
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.
it is already there in
ScalaAkkaClientCodegen extends AbstractScalaCodegen extendsDefaultCodegen base class.I will add unit test to check there is no breaking changes for the further versions