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

Save reason phrase in Protocol::HTTP1::Response #146

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

uberjay
Copy link
Contributor

@uberjay uberjay commented Oct 23, 2023

Save away the raw reason phrase provided by the server. This is of limited niche value, as the reason phrase seems to be mostly unused/ignored.

One example of where it's needed:

The Proxmox VE API ships error reason strings back using the HTTP reason phrase. Without this side-channel, error status is 100% opaque/unavailable.

See (very brief) discussion: #145

It's probably not worth overthinking this, but some other thoughts that came to mind were:

  • Does it make sense to define a reason accessor in other HTTP version responses so the method is always available?
    • My opinion: no, not worth it and feels pretty bad to burden other versions of HTTP with this detail when it's nearly irrelevant.
  • Is it worth any sort of string optimization scheme? (e.g., String#dedup.)
    • My opinion: probably not, but happy to hear why it's a good idea.

Types of Changes

  • New feature.

Contribution

@uberjay uberjay force-pushed the http1-reason-phrase branch from 44c1e8a to 12e4e1e Compare October 23, 2023 16:06
Save away the raw reason phrase provided by the server. This is of
limited niche value, as the reason phrase seems to be _mostly_
unused/ignored.

One example of where it's needed:

The [Proxmox VE API](https://pve.proxmox.com/wiki/Proxmox_VE_API)
ships error reason strings back using the HTTP reason phrase. Without
this side-channel, error status is 100% opaque/unavailable.
@uberjay uberjay force-pushed the http1-reason-phrase branch from 12e4e1e to 5ed9fd6 Compare October 23, 2023 16:15
@ioquatix ioquatix merged commit a6f0617 into socketry:main Oct 24, 2023
13 of 18 checks passed
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.

2 participants