Changed Response to use body: bytes #375
Conversation
- Added ActixBytesWrapper to conform to pyclass and MessageBody
✅ Deploy Preview for robyn canceled.
|
|
Thank you for the PR @madhavajay 😄 The pr looks nice from the first glance but I will do a thorough review tomorrow morning 😄 |
There was a problem hiding this comment.
Hi @madhavajay, and thanks for this PR!
I found a way to fix the issue you had with breaking compatibility with str return type. I left some suggestions that will make your code compatible with both bytes and str.
The ActixBytesWrapper struct is actually very useful and it could probably help with other features in the future too, good job on this!
|
@sansyrox feel free to add your comments too if you have some, I focused mostly on the |
There was a problem hiding this comment.
Hey @madhavajay 👋
Thank you for your PR. The ActixBytesWrapper is a very helpful struct 😄 and thank you @AntoineRR for the review 😄
I have a few inline comments in addition to @AntoineRR 's review
- Reverted unnecessary casting and type changes
- WIP: Issue with async testing
|
@AntoineRR and @sansyrox thanks for the feedback. I have made the changes. I thought it might be a good idea to include a bad input test but the async one doesn't await and i'm not sure how to get |
|
That's a good idea to include a test for bad input. It should however be done without the |
|
@AntoineRR I tried but it seems like the exceptions aren't catchable right now so I think ill just remove this so we can merge and then this can be addressed separately.
|
- Added tests for bad and good body types
- Parameterized body test fixtures
There was a problem hiding this comment.
This looks good to me 😄
@AntoineRR , do you want to give it a final glance?
|
LGTM 👍 |
|
@madhavajay , there's just one final conflict. Everything else looks good 😄 |
|
The conflict was with my test refactor, I solved it from Github 🙂 |
|
Thank you @madhavajay and @AntoineRR . Great work 😄 |
Description
This PR fixes #374
I know that my code is not optimal, but I wanted to see what the problem was and if i could get it to work.
I don't have a lot of experience with Rust or Maturin / pyo3 and I get really confused on the lifetimes, copying of bytes etc.
Was there a different goal in mind for allowing Robyn to return bytes?
One thing that is a problem is, if the Response() struct
bodycontains a string I can't detect it in the_format_responsemethod because the rust struct has no getters. This means a python user can use a string in body and it will fail.It would be good to be able to detect this, or maybe the Rust struct needs to take a Union type and detect the string and convert it then?
I look forwards to feedback on my PR! 🦀