Skip to content
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

Merged
merged 2 commits into from
Apr 3, 2023
Merged

docs: Add docs for v0.26.0 #451

merged 2 commits into from
Apr 3, 2023

Conversation

sansyrox
Copy link
Member

@sansyrox sansyrox commented Mar 25, 2023

Description

Fixes #454

Add documentation for the latest release

@sansyrox sansyrox requested a review from AntoineRR March 25, 2023 02:14
@sansyrox sansyrox marked this pull request as ready for review March 25, 2023 02:15
@ghost
Copy link

ghost commented Mar 25, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 25, 2023

CodSpeed Performance Report

Merging #451 docs/v0.26.0 (c0508d5) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 74 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

Copy link
Collaborator

@AntoineRR AntoineRR left a 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 Show resolved Hide resolved
docs/features.md Outdated
Comment on lines 228 to 235
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
```
Copy link
Collaborator

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.

Copy link
Member Author

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 😅

Copy link
Member Author

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?

Copy link
Collaborator

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

Copy link
Member Author

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 😅

docs/features.md Outdated Show resolved Hide resolved
docs/features.md Outdated Show resolved Hide resolved
@sansyrox
Copy link
Member Author

Thanks for the review @AntoineRR 😄

I just have a question.

@sansyrox sansyrox requested a review from AntoineRR April 1, 2023 15:01
@sansyrox sansyrox force-pushed the docs/v0.26.0 branch 2 times, most recently from c178e6c to 52eca16 Compare April 1, 2023 15:18
@sansyrox
Copy link
Member Author

sansyrox commented Apr 3, 2023

Merging as want to release v0.26.1

@sansyrox sansyrox merged commit 27c29c2 into main Apr 3, 2023
@sansyrox sansyrox deleted the docs/v0.26.0 branch April 3, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation for robyn.env file
2 participants