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

Response context RFC #22

Merged
merged 19 commits into from
Oct 7, 2022
Merged

Response context RFC #22

merged 19 commits into from
Oct 7, 2022

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Oct 3, 2022

Add Response interface that contains information on a HTTP response related to the event.

Rendered RFC

@marandaneto marandaneto marked this pull request as ready for review October 3, 2022 08:05
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

I have a slight preference for Option 2 -- I feel like event.request itself should be a context and is only a top-level field for historical reasons. I do not have a strong opinion though, both options seem fine to me.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Between Option 1 and Option 2, I would recommend to go with Option 2: Adding a Response context type in contexts. This allows us to create typing for known fields, documentation, normalization, and shows up in the UI automatically. When the SDK is used with older self-hosted deployments, this further does not lead to errors showing up in the issue details UI.

Once we've chosen an option, please structure the RFC so that it describes the proposed change and lists alternatives separately.

Co-authored-by: Joris Bayer <jjbayer@gmail.com>
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Good to go once the comment on on is_redirect is resolved. Thank you!

@marandaneto marandaneto merged commit cdabf01 into main Oct 7, 2022
@marandaneto marandaneto deleted the rfc/response-context branch October 7, 2022 07:39
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.

4 participants