-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[Ruby] Incorrect return types if multiple responses are defined #7634
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
base: master
Are you sure you want to change the base?
[Ruby] Incorrect return types if multiple responses are defined #7634
Conversation
…to feature/process-multiple-requests
@spacether could you please check this PR? Thanks! |
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.
Looks good to me. Thank you for the PR!
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 you fix the travis ci errors?
There are failing ruby tests there.
I'm confused, because these tests are failed not because of my changes. It fails even on master branch. Can you take a look? @spacether |
Hi, any updates on this? |
return_type = opts[:debug_return_type] | ||
|
||
return_types_map = { | ||
200 => 'Array<Pet>', |
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 map value is incorrect. Can you update it to Array<Pet>
?
|
||
return_types_map = { | ||
200 => 'Array<Pet>', | ||
400 => '', |
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.
Should the value be empty string or nil?
new_options = opts.merge( | ||
:operation => :"{{classname}}.{{operationId}}", | ||
:header_params => header_params, | ||
:query_params => query_params, | ||
:form_params => form_params, | ||
:body => post_body, | ||
:auth_names => auth_names, | ||
:return_type => return_type | ||
:return_type => return_type, | ||
:return_types_map => return_types_map |
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.
You are using two parameters to pass in the same data here:
- return_type
- return_types_map
Both of these parameters are saying deserialize a response into this class.
How about only passing in return_types_map?
It looks the old code replace the return type with the debug_return_type if it is passed in.
If the user sets a :debug_return_type should we set all values of the map to :debug_return_type?
Also if a different status code is received and debug_return_type is passed in, what type should be used? If we want to use debug_return_type then we should pass it in as the initialization argument to Hash.new so it will be used for missing entries.
One could add this missing feature with:
return_types_map = Hash.new(opts[:debug_return_type])
if opts[:debug_return_type] != nil:
# add the code to classname value here
# otherwise we keep the hash as-is and all access of it grabs the default value
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.
Thanks for this PR overall it looks good.
- One mapped value is incorrect and needs a fix.
I have a questions about:
- using empty string vs nil
- asking if we can use one parameter to store the data types rather than two for simplicity
Description
If you define multiple responses, the codegen will only support the first one (typically 200).
openapi-generator version
4.3.1 and higher
OpenAPI declaration file content or url
Similar Issue for PHP
https://github.com/OpenAPITools/openapi-generator/issues/125
Similar Issue for Python
https://github.com/OpenAPITools/openapi-generator/issues/7426