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

feat: Convert the returned dictionaries directly to json #406

Closed
wants to merge 8 commits into from

Conversation

mikaeelghr
Copy link
Contributor

Description
This would be a breaking change.

This PR fixes #393

@netlify
Copy link

netlify bot commented Feb 11, 2023

Deploy Preview for robyn canceled.

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

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.

@mikaeelghr , I was thinking about this PR for a while and I believe I have an approach that will not break anything.

We can have two ways:

  1. First, we can try casting dictionary to a Response.

  2. If that doesn't work, we can return it as a json body.

Or, the second way is that we can directly allow dictionaries in the body along with string and bytes.

What do you think?

cc: @AntoineRR

@mikaeelghr
Copy link
Contributor Author

mikaeelghr commented Feb 18, 2023

Thanks for your comment @sansyrox.

In my opinion, your first option is pretty but it's a complex behavior and may confuse the developer. For example consider the situation that the developer needs to send Json response with a similar schema to { "status_code", "body", "type", "headers" }.

Your second option is also a good option.

BTW I think maybe with some startup configuration we can use your first method and deprecate casting dictionary to the Response object after a while.

Something like this (of course with better naming):

app = Robyn(_ file _, map_dictionary_to_json=True)

@AntoineRR
Copy link
Collaborator

I second @mikaeelghr on the first option, it is confusing and would prevent returning json with some special keywords. I would go for the following solution :

  • returning a dictionary converts it to json, even if it has a status_code or other body field.
return {
    "field": "This is a wonderful json body",
    "body": "This does not prevent returning json"
}
  • If some special fields of the response must be specified (for example headers), user should return a Response struct with the info he wants and a dict body:
return Response(
    headers = { "whatever": "header" },
    body = {
         "field": "This is a wonderful json body",
         "body": "This does not prevent returning json"
    }
)

I think at this stage we can afford breaking changes like this as Robyn hasn't reached a stable API yet. This is what 0.x versions are for after all! 🙂

A few pointers I have in mind so far to implement this:

  • Detect if we return a dict body or if the body of a Response object is a dict and add a the following header to the response in these cases:
Content-Type: application/json
  • Change the ActixBytesWrapper struct to accept a PyDict too

There are probably things missing but I think this is the general idea. Does it seem ok to you two?

@sansyrox
Copy link
Member

@mikaeelghr @AntoineRR , thank you for you comments.

I agree with both of you. @mikaeelghr , at this stage we can afford breaking changes and we don't need to worry about slow migrations.

@AntoineRR , I basically agree with all the points you said 😄

And this way we can finally get rid of the type=text in the Response class.

@sansyrox
Copy link
Member

This has been implemented.

@sansyrox sansyrox closed this Apr 22, 2024
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.

[Feature Request] Convert the returned dictionaries directly to json?
3 participants