Skip to content

Conversation

@meck-gd
Copy link

@meck-gd meck-gd commented Feb 10, 2023

Some APIs may acknowledge requests with status 204 No Content and they won't provide a response body.

In particular, for server responses such as 204 No Content, json()
must not be called because it would fail.
Copy link
Owner

@MarcoMuellner MarcoMuellner left a comment

Choose a reason for hiding this comment

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

Please change the templates accordingly, the nesting makes it fairly hard to read. Otherwise thank you for fixing the issue :)

Copy link
Owner

@MarcoMuellner MarcoMuellner left a comment

Choose a reason for hiding this comment

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

Probably need to discuss what to return in this case.

{% if return_type.complex_type %}
{% if return_type.list_type is none %}
{% if return_type.type is none or return_type.type.converted_type is none %}
return response.json()
Copy link
Owner

Choose a reason for hiding this comment

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

I think return None is the correct approach here, instead of the dictionary, as the return type is also typed as None in this case afaik. It should be also in line to what the Openapi definition actually provides.

Copy link
Author

Choose a reason for hiding this comment

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

My bad, that is actually what I originally intended. I got confused by the logic inversion. Fixed the commit.

@meck-gd meck-gd force-pushed the fix-returntype-none branch from 5ef1d62 to 27421c4 Compare February 22, 2023 17:11
@MarcoMuellner
Copy link
Owner

Thanks for the PR :)

@MarcoMuellner MarcoMuellner merged commit cc7c151 into MarcoMuellner:main Feb 23, 2023
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.

2 participants