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

chore(http): Refresh headers docs (d-k) #36075

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

bsmth
Copy link
Member

@bsmth bsmth commented Sep 27, 2024

Description

This PR refreshes a few HTTP headers pages.

common changes:

  • Improve See also
  • Capitalize Yes/No in properties table
  • Normalize The HTTP **`Header-Name`** (request|response) header in first sentence
  • Add status text to HTTPStatus macros, e.g. {{HTTPStatus("406", "406 Not Acceptable")}}
  • Remove improper use of quotes in and around code
  • Avoid escaping angle brackets in directive names in favor of backticks:
- \<directive>
+ `<directive>`

Motivation

Keeping content up-to-date, fixing formatting, unifying writing conventions

Related pull requests:

@bsmth bsmth requested a review from a team as a code owner September 27, 2024 09:17
@bsmth bsmth requested review from Elchi3 and removed request for a team September 27, 2024 09:17
@github-actions github-actions bot added Content:HTTP HTTP docs size/l [PR only] 501-1000 LoC changed labels Sep 27, 2024
Copy link
Contributor

github-actions bot commented Sep 27, 2024

Preview URLs (22 pages)
Flaws (1)

Note! 21 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/HTTP/Headers/Digest
Title: Digest
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: http.headers.Digest
External URLs (7)

URL: /en-US/docs/Web/HTTP/Headers/Expect-CT
Title: Expect-CT


URL: /en-US/docs/Web/HTTP/Headers/Keep-Alive
Title: Keep-Alive


URL: /en-US/docs/Web/HTTP/Headers/DPR
Title: DPR


URL: /en-US/docs/Web/HTTP/Headers/If-None-Match
Title: If-None-Match

(comment last updated: 2024-10-29 17:06:07)

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Oct 24, 2024
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@@ -7,12 +7,10 @@ browser-compat: http.headers.Date

{{HTTPSidebar}}

The **`Date`** general HTTP header contains the date and time
at which the message originated.
The HTTP **`Date`** header contains the date and time at which the message originated.
Copy link
Collaborator

@hamishwillee hamishwillee Oct 24, 2024

Choose a reason for hiding this comment

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

This form is used in some places. Up to you. I like the consistency.

Suggested change
The HTTP **`Date`** header contains the date and time at which the message originated.
The HTTP **`Date`** {{Glossary("Request header","request")}} and {{Glossary("Response header","response")}} header contains the date and time at which the message originated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was in two minds about this, my reasoning was it's either "requests only" or "responses only" (or a more granular classification), otherwise it's "a header" when it can be either, so I'm leaning towards leaving these out in those cases, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually here's one that landed recently: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-WebSocket-Version

So let's go with this convention 👍🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the convention because you're explicitly saying if it can be a request, response, or both. Without mentioning both, you could have just forgotten to add the information. Tiny bit better for reader.

@@ -15,10 +15,11 @@ browser-compat: http.headers.Digest
> Use {{HTTPHeader("Content-Digest")}} instead.
> For `id-*` digest algorithms, use {{HTTPHeader("Repr-Digest")}}.

The **`Digest`** response or request HTTP header provides the other side with a {{Glossary("digest")}} of the {{HTTPHeader("Content-Encoding")}}-encoded _selected representation_. It can be requested by using the {{HTTPHeader("Want-Digest")}} header.
The HTTP **`Digest`** header provides the recipient with a {{Glossary("digest")}} of the {{HTTPHeader("Content-Encoding")}}-encoded _selected representation_.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, I like this form ...

Suggested change
The HTTP **`Digest`** header provides the recipient with a {{Glossary("digest")}} of the {{HTTPHeader("Content-Encoding")}}-encoded _selected representation_.
The HTTP **`Digest`** {{Glossary("Request header","request")}} and {{Glossary("Response header","response")}} header provides the recipient with a {{Glossary("digest")}} of the {{HTTPHeader("Content-Encoding")}}-encoded _selected representation_.

@@ -7,12 +7,10 @@ browser-compat: http.headers.Date

{{HTTPSidebar}}

The **`Date`** general HTTP header contains the date and time
at which the message originated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that the change you have made puts all the content up the top. I am happy with that, but in some case we might have too much information. So for the template I'd suggest that if the intro goes over the fold the author is allowed to add a separate section "Description" after the directive and before the direction, moving most of the content to that section.

That somewhat mirrors what Web APIs do.

Copy link
Member Author

Choose a reason for hiding this comment

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

add a separate section "Description" after the directive and before the direction

I don't follow this one 😄, where do you think "Description" should go?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So in Web APIs this appears after syntax sections, before Examples. The template hints that you have a short introductory sentence and then all the detail in the description. I don't particularly like that and try to avoid a description section if I can keep the text up the top succint.

So this pattern for HTTP would be:

image

Open to discussion. We don't have to do this, we just need to make sure that all this work you're doing is captured.


Servers that opt in to the `DPR` client hint will typically also specify it in the {{HTTPHeader("Vary")}} header to inform caches that the server may send different responses based on the header value in a request.

> [!WARNING]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this warning should perhaps live up the top with the other headers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can put it up top, but we're getting pretty strong banner overload on this page now:

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely correct. I don't think that matters because this is deprecated and we don't really want people to read it anyway :-).

That said, hopefully Ruth will eventually get the proposal for banners sorted out and implemented.

The **`Early-Data`** header is set by
an intermediary to indicate that the request has been conveyed in [TLS early data](/en-US/docs/Web/Security/Transport_Layer_Security#tls_1.3),
and also indicates that the intermediary understands the {{HTTPStatus("425", "425 Too Early")}} status code.
The HTTP **`Early-Data`** {{Glossary("request header")}} is set by an intermediary to indicate that the request has been conveyed in [TLS early data](/en-US/docs/Web/Security/Transport_Layer_Security#tls_1.3), and also indicates that the intermediary understands the {{HTTPStatus("425", "425 Too Early")}} status code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I don't really "get" this, but I am pretty sure that it doesn't matter - this is sufficient for most readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can add some details to this page: https://aws.github.io/s2n-tls/usage-guide/ch14-early-data.html

Copy link
Member Author

Choose a reason for hiding this comment

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

I also noticed there's no See also section here

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. I don't feel very strongly either way. It feels like the kind of thing only a deep dark expert would need, and they would read the spec.

@Elchi3 Elchi3 removed their request for review October 28, 2024 12:41
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Oct 29, 2024
@bsmth bsmth requested a review from a team as a code owner October 29, 2024 17:04
@bsmth bsmth requested review from hamishwillee and removed request for a team October 29, 2024 17:04
@github-actions github-actions bot added the Content:Glossary Glossary entries label Oct 29, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

Missing a See also section on this page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Glossary Glossary entries Content:HTTP HTTP docs size/l [PR only] 501-1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants