-
-
Notifications
You must be signed in to change notification settings - Fork 247
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: add support nested routes prefix and multiple auth handler #889
Conversation
@yomaaf is attempting to deploy a commit to the sparckles Team on Vercel. A member of the Team first needs to authorize it. |
Hey @sansyrox please check this out. I made some adjustment to enable nested routes |
Hey @yomaaf 👋 Thank you for the PR 😄 I will have a look soon. @VishnuSanal , could you have an initial look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Please let me know if this code need some adjustment @sansyrox @VishnuSanal, because I'm also working on #708 issue based on this code. I found out your framework and want to use it on production. But need this nested function and multiple authentication. I read the comment on #708 issue and have different approach. Will PR soon. |
Hey @yomaaf 👋 Thanks for the PR. I am with limited access till Sunday. I will do a thorough review by Sunday 😄 |
robyn/__init__.py
Outdated
def include_router(self, router, added_routes=None): | ||
""" | ||
The method to include the routes from another router | ||
|
||
:param router Robyn: the router object to include the routes from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need an added_route
param here? Could you please the usage in the docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add the typing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I want to comment in this conversation but reply on another conversation instead. My mistake.
#889 (comment)
self.sub_router = [] | ||
self.temp_routes = [] # Temporary storage for routes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the idea behind these memebers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi I'm sorry there is some issue before when register route. I already fix this. Please check again. I also add some feature to handle multiple auth handler like I said in comment before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also add some pytest to ensure the function is working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is we save all the nested subrouter and route before the main robyn call include router. Like what I explain in this comment #889 (comment)
robyn/__init__.py
Outdated
def get(self, endpoint: str, const: bool = False, auth_required: bool = False): | ||
def inner(handler): | ||
self.add_temp_route(HttpMethod.GET, endpoint, handler, const, auth_required) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why did you have to refactor the original implementation here?
^^ I am trying to understand you thoughts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original implementation of the get, post, etc., decorators calls super(), which registers the route when the decorator is used. However, when nested subrouters are used, the decorator has already been called, so the route retains the old prefix instead of adopting the nested prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original implementation of the get, post, etc., decorators calls super(), which registers the route when the decorator is used. However, when nested subrouters are used, the decorator has already been called, so the route retains the old prefix instead of adopting the nested prefix.
Could you please add it in comment/docstring in the codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. I'll add it.
Hey @yomaaf , I have a few questions. |
* add multiple auth handler * add basic authorization getter * add route params auth handler name
CodSpeed Performance ReportMerging #889 will not alter performanceComparing Summary
Benchmarks breakdown
|
Hey @yomaaf 👋 Thanks for the PR but this PR is doing a lot of things 😅 Could you split the PR in two parts
I am recommending the above as it will take a lot of time to get it landed 😄 |
Hi, @sansyrox. I will do that. I'll separate the PR, but maybe not today. Will do in a few days because I need to catch up on another project. In the meantime, I'll create my own branch in my repository and use pip install from it till the PRs land. I'm going to push it as soon as possible. Thanks |
Description
This PR fixes #865 #708
This PR adds the ability to nested route and ability multiple auth handler.
The example already added to integration tests