fix: factorizing code #322
Conversation
✅ Deploy Preview for robyn canceled.
|
|
Hey @AntoineRR 👋 Thanks for your PR. I didn't have much time to review your PR over the past few days. I will try reviewing it tomorrow. Apologies for the delay. 😄 |
|
No worries, take as much time as you need ^^ |
robyn/processpool.py
Outdated
| server.add_route( | ||
| route_type, endpoint, handler, is_async, number_of_params, const | ||
| ) | ||
| route_type, endpoint, function, const = route |
There was a problem hiding this comment.
If we are refactoring this code, can we please rename the const variable to is_const or is_const_function?
There was a problem hiding this comment.
Makes sense, I changed it to is_const where it is relevant.
| route_type, endpoint, function = route | ||
| server.add_middleware_route(route_type, endpoint, function) |
There was a problem hiding this comment.
Also, I love how the routes and the functions are kept segregated ✨
| pub struct Request { | ||
| pub queries: HashMap<String, String>, | ||
| pub headers: HashMap<String, String>, | ||
| pub method: Method, | ||
| pub params: HashMap<String, String>, | ||
| pub body: Bytes, | ||
| } |
There was a problem hiding this comment.
@AntoineRR what method does it give to the request object on Request:default()?
There was a problem hiding this comment.
From actix_web:
impl Default for Method {
#[inline]
fn default() -> Method {
Method::GET
}
}
The default method is a GET then. It shouldn't matter for the const router, I just wanted a request to use the execute_http_function method, but it shouldn't be used in the case of the const router
| @@ -44,7 +42,7 @@ impl Router<String, Method> for ConstRouter { | |||
| event_loop.context("Event loop must be provided to add a route to the const router")?; | |||
|
|
|||
| pyo3_asyncio::tokio::run_until_complete(event_loop, async move { | |||
| let output = execute_function(function, number_of_params, is_async) | |||
| let output = execute_http_function(&Request::default(), function) | |||
There was a problem hiding this comment.
You're welcome ;)
I think it makes sense to only have one method to execute the function, as we will only have to change this one for an impact on HttpRouter and ConstRouter at the same time!
|
All looks good from the first glance. I have one last question - is I will just try a final run and test before approving 😄 Great work 👍 ✨ |
|
I'm glad you like it! |
Description
This PR is a big refactor of Robyn's core. It aims at simplifying the code by factorizing it and introducing new useful structs. Here is a description of what I've done to achieve this:
Requeststruct that wraps the parameters of a request (queries,headers,method,params, andbody)Requeststruct in request handlers and executors instead of using each parameters of a request separatelyFunctionInfostruct to describe functions coming from Python. This struct is made available in the Python code too and wrapshandler,is_asyncandnumber_of_paramsFunctionInfostruct replacehandler,is_asyncandnumber_of_paramsin every function callexecute_functionfunction that was used by the const router has been removed as it was very similar toexecute_http_function. Const router now usesexecute_http_function(not sure about this one, let me know if I should restoreexecute_functionwhich usedtokio::task::spawn_blockingwhileexecute_http_functiondoesn't)This a big PR and it affects a lot of modules. It may need some time to be fully reviewed, do not hesitate to ask me questions about it and ask me to restore some parts if I made mistakes :)