-
-
Notifications
You must be signed in to change notification settings - Fork 253
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
docs: Add docs for v0.26.0 #451
Conversation
CodSpeed Performance ReportMerging #451 Summary
|
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.
Hey @sansyrox , thanks for updating the docs. I left a few comments.
Also I think there are other parts in the docs where we access body
, params
and queries
. Those should change as well. I found some others in the features.md
file, but also graphql-integration.md
. Can you update them too please?
docs/features.md
Outdated
Additionally, you can access headers for per route. | ||
|
||
```python | ||
@app.get("/test-headers") | ||
def sync_before_request(request: Request): | ||
request.headers["test"] = "test-header" | ||
return request | ||
``` |
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.
This will not work I think. In a route, we want to return a Response or the body of a response. If we want to change the headers of a request object, it is only relevant in a before middleware.
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.
This will not work I think. In a route, we want to return a Response or the body of a response. If we want to change the headers of a request object, it is only relevant in a before middleware.
@AntoineRR , what you mean by this will not work? Yep, look at it, it is a bad example. But is working from looking at the code 😅
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.
Are you suggesting that we should change the example 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.
@sansyrox I just tested this and it does not crash, although the response we get from the server is something like <builtins.Request object at 0x7f63d56f2330>
(which is the result of str(request)
.
Also, there really is no point currently in modifying the request headers in a route, because we will not use the request in the rest of the code afterwards. Route methods take Request
as an input and returns Response
. The only part were it makes sense to modify request headers is in a before middleware, because the modified Request
object will then be used inside the route method. This example would be more relevant like this IMO:
@app.before_request("/test-headers") # Note this is a middleware
def test_headers_before_request(request: Request):
request.headers["test"] = "test-header"
return request
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.
@AntoineRR , the reason this example exists is to showcase the possibility. The ability to access headers in middlewares is shown in a section below.
I am not concerned if it is a sensible thing to do or not, I just want to document that you can do it in Robyn.
I hope this makes sense 😅
Thanks for the review @AntoineRR 😄 I just have a question. |
c178e6c
to
52eca16
Compare
for more information, see https://pre-commit.ci
Merging as want to release v0.26.1 |
Description
Fixes #454
Add documentation for the latest release