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

fix: better handling of route return type #349

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

AntoineRR
Copy link
Collaborator

Description

I figured there are issues when you try to return something other than a string or a dict in a route. With this PR, you can return anything and most notably directly a Response.
We may want to add it to the doc if it seems relevant to you @sansyrox ?

@netlify
Copy link

netlify bot commented Jan 5, 2023

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit 7485364
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/63bdc2eb2004fa00087ce3a5

@sansyrox
Copy link
Member

sansyrox commented Jan 5, 2023

@AntoineRR , thank you for your PR. Can we still return a dictionary in this PR?

@AntoineRR
Copy link
Collaborator Author

Yes sure!
Before, we could return only a str or dict, now you can still do that but also return an int or a Response for example.

Copy link
Member

@sansyrox sansyrox left a 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! 🔥

I am just wondering where will we add the documentation regarding this 🤔

@AntoineRR
Copy link
Collaborator Author

Hey @sansyrox, thanks for your review. I added docs regarding the types returned from a route in the features section. I think it fits here, is it ok for you?

Comment on lines +102 to +117
return {
"status_code": 200,
"body": "This is a regular response",
"type": "text",
"headers": {"Header": "header_value"},
}
Copy link
Member

@sansyrox sansyrox Jan 8, 2023

Choose a reason for hiding this comment

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

Hey @AntoineRR ,

The return type of json should be

return {
        "status_code": 200,
        "body":  jsonify({"hello": "world"},
        "headers": {"Content-Type": "application/json"},
    }

or just

return jsonify({"hello": "world"})

It will be returning a Plain Text otherwise

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh my bad you're right, I will change this :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sansyrox it should be ok now 🙂

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

LGTM! Great work! 🔥

@sansyrox sansyrox merged commit 4eb4786 into sparckles:main Jan 11, 2023
kliwongan added a commit to kliwongan/robyn that referenced this pull request Jan 13, 2023
fix: better handling of route return type (sparckles#349)
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