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.

Sign up for GitHub

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

Jump to bottom

feat: Allow global level Response headers #410

Merged
merged 2 commits into from
Feb 25, 2023
Merged

feat: Allow global level Response headers #410

merged 2 commits into from
Feb 25, 2023

Conversation

ParthS007
Copy link
Contributor

@ParthS007 ParthS007 commented Feb 12, 2023 •
edited
Loading

Description

This PR fixes #335

  • Add support for global level response headers
  • @sansyrox We can merge add_response_header & add_request_header and remove_header & remove_response_header?

@netlify
Copy link

netlify bot commented Feb 12, 2023 •
edited
Loading

✅ Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit ef83024
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/63f94dc35515150008bf7896

@sansyrox
Copy link
Member

Thanks for the PR @ParthS007 . Great work like always! 😄

We can merge add_response_header & add_request_header and remove_header & remove_response_header?

What do you mean by this?

@ParthS007
Copy link
Contributor Author

We can merge add_response_header & add_request_header and remove_header & remove_response_header?

What do you mean by this?

Both the methods add_response_header and add_request_header have similar functionality, we can use only one method for adding and have a keyword argument specifying either to insert in global_response or global_request header. Same goes in removal of header.

Does this sounds clear?

@sansyrox
Copy link
Member

Does this sounds clear?

Ah, that makes sense. We can do that in the rust side of code. But for the python facing developer API, having separate functions allows the developers to make fewer errors. As in Robyn, we follow explicit functions for a lot of things.

Would you like to work on this(integrating the rust side) btw?

@ParthS007
Copy link
Contributor Author

@sansyrox I can help with that, Can you please point me to the related code in source and I will start having a look.

Is there anything else to add in this PR?

@sansyrox
Copy link
Member

@ParthS007 , I was talking about this function(https://github.com/sansyrox/robyn/pull/410/files#diff-48d27698cc708c7efe770fd897e4885efaa0a15cae30d54936068603ba03bbd9R241) and its equivalent.

But I have been thinking about this. Do you think this optimization is necessary for a two-line function? Would a separation make more sense?

@sansyrox
Copy link
Member

Is there anything else to add in this PR?

@ParthS007 ,

I think we can merge this 😄

sansyrox
sansyrox approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work! 😄 ✨

@ParthS007
Copy link
Contributor Author

ParthS007 commented Feb 15, 2023 •
edited
Loading

@ParthS007 , I was talking about this function(https://github.com/sansyrox/robyn/pull/410/files#diff-48d27698cc708c7efe770fd897e4885efaa0a15cae30d54936068603ba03bbd9R241) and its equivalent.

But I have been thinking about this. Do you think this optimization is necessary for a two-line function? Would a separation make more sense?

@sansyrox yeah okay. I think separation is good, more clear like this. 👍

@sansyrox
Copy link
Member

@ParthS007 , a few tests are failing. Can you please rebase to the latest branch?

@sansyrox sansyrox mentioned this pull request Feb 15, 2023
Closed
sansyrox
sansyrox requested changes Feb 16, 2023
View reviewed changes
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ParthS007 ,

With the change in definition of spawn_process, we will also have to change the args here(https://github.com/sansyrox/robyn/blob/09e24ef6ee1b64e89a277e17b72303bb86a4f9bc/robyn/processpool.py#L72) and here(https://github.com/sansyrox/robyn/blob/09e24ef6ee1b64e89a277e17b72303bb86a4f9bc/robyn/processpool.py#L88)

This is why the tests are failing

ParthS007
ParthS007 commented Feb 19, 2023
View reviewed changes
src/types.rs
@@ -164,6 +164,7 @@ pub struct Response {

#[pymethods]
impl Response {
Copy link
Contributor Author

@ParthS007 ParthS007 Feb 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sansyrox We need to make change here to check for response headers and create the Response accordingly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ParthS007 , in the future yes. But would we need that for now too?

I suppose you are suggesting for things like json content?

ParthS007
ParthS007 commented Feb 19, 2023
View reviewed changes
docs/features.md
@@ -194,6 +202,14 @@ async def request_headers():
}
```

```python
@app.get("/response_headers")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have similar endpoint for response_header as for request_header?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ParthS007 , we already have one here(https://github.com/sansyrox/robyn/pull/410/files#diff-aff15ced1fe37506f41f2e1d93a4d8312529259f7803f8634ffb5eac4de47823R196)

Is this the same one that you're suggesting?

sansyrox
sansyrox approved these changes Feb 24, 2023
View reviewed changes
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can merge and then work on this

@sansyrox sansyrox merged commit 91ffd1c into sparckles:main Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sansyrox sansyrox sansyrox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Allow global level Response headers
2 participants
@ParthS007 @sansyrox