add route type enum #299
add route type enum #299
Conversation
✅ Deploy Preview for robyn canceled.
|
src/routers/middleware_router.rs
Outdated
| @@ -9,6 +9,8 @@ use matchit::Node; | |||
|
|
|||
| use anyhow::{bail, Error, Result}; | |||
|
|
|||
| use super::router::RouteType; | |||
There was a problem hiding this comment.
I don't think super is required anymore in Rust.
There was a problem hiding this comment.
another option is to write like this.
use crate::routers::types::MiddlewareRouteType;
There was a problem hiding this comment.
@suhailmalik07 , yep. That is the preferred way.
There was a problem hiding this comment.
update with this.
src/routers/middleware_router.rs
Outdated
| fn get_relevant_map(&self, route: &str) -> Option<&RwLock<Node<(PyFunction, u8)>>> { | ||
| fn get_relevant_map( | ||
| &self, | ||
| route: MiddlewareRouteType, |
There was a problem hiding this comment.
just one last nit: MiddlewareRoute would make more sense than MiddlewareRouteType.
Reason:
If you ask the type of route then it should be MiddlewareRoute and not type = MiddlewareRouteType.
There was a problem hiding this comment.
Sure, updated this also.
There was a problem hiding this comment.
Thanks again @suhailmalik07 for the quick updates. I just have two final suggestions before we can merge this PR 😄
|
@suhailmalik07 , there are some conflicting files. Can you please rebase the latest changes? |
suhailmalik07
force-pushed
the
refactoring
branch
from
50abf6e to
299b1e2
Compare
Rebased 🙂 |
Description
This PR fixes #254
I'm working on this.
@sansyrox It will be great, If you can point me to the places from where you think code can be converted to enum and types?