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

doc,http,http2: add http highlight grammar #33785

Closed
wants to merge 1 commit into from
Closed

doc,http,http2: add http highlight grammar #33785

wants to merge 1 commit into from

Conversation

DerekNonGeneric
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric commented Jun 7, 2020 •
edited
Loading

Prior to this commit, http request message code blocks in Markdown
files were not being highlighted correctly. This has been corrected by
adding the new grammar to the bundle, removing the CRLFs (\r\n) from
these code samples, adding a reminder to re-add them, and tuning the
syntax theme to support attribute highlighting.

Checklist

image

/cc @Trott

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 7, 2020
@DerekNonGeneric
Copy link
Contributor Author

CI failure is unrelated to these changes.

/cc @nschonni

@Trott
Copy link
Member

Trott commented Jun 8, 2020

Fix for CI failure: #33789

nschonni
nschonni reviewed Jun 9, 2020
View reviewed changes
doc/api_assets/hljs.css
@@ -7,6 +7,7 @@
color: #333;
}

.hljs-attribute,
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to have any unintended affect on the other existing code blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, these are the only two instances I was able to find. Something that does not yet have highlighting is the .hljs-attr class, which probably needs to happen at some point, but this is not the same as that.

@DerekNonGeneric
Copy link
Contributor Author

Since we removed the newline escapes, it might be necessary to add a small explanation as Mozilla has done in the following page section.

https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers#Server_handshake_response

(remember each header line ends with \r\n and put an extra \r\n after the last one to indicate the end of the header)

/cc @Trott

@Trott
Copy link
Member

Trott commented Jun 15, 2020

Since we removed the newline escapes, it might be necessary to add a small explanation as Mozilla has done in the following page section.

https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers#Server_handshake_response

(remember each header line ends with \r\n and put an extra \r\n after the last one to indicate the end of the header)

/cc @Trott

Emulating MDN on this is a good idea IMO.

@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Jun 16, 2020 •
edited
Loading

Does anyone from @nodejs/http or @nodejs/http2 want to comment on this? I really don't know whether or not that small explanation is applicable to both documents or even a correct suggestion. It may be possible that MDN is wrong too. Should there be a terminating CRLF? Should both of these examples be run with having them? I found some good information here too. I'm a bit concerned about what's stated in https://tools.ietf.org/html/rfc7230#section-3.5 and don't really understand the meaning.

I would consider this PR stalled until someone can say for certain. If the wait-for-feedback label proposed by @addaleax in #33879 existed, I would request adding it at this point. :)

@DerekNonGeneric DerekNonGeneric marked this pull request as draft June 16, 2020 03:12
mcollina
mcollina requested changes Jun 16, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I think adding an explanation on \r\n is necessary. Otherwise this looks good.

@DerekNonGeneric DerekNonGeneric marked this pull request as ready for review June 28, 2020 05:43
@DerekNonGeneric DerekNonGeneric requested a review from mcollina June 28, 2020 05:43
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Jun 28, 2020 •
edited
Loading

@mcollina, sorry for the delay on this, I kind of forgot about it. Would you mind re-reviewing it, please?

I've added a reminder to re-add the CRLFs. Adding further explanation might be beyond the scope of this PR.

Please let me know what you think!

doc: add http highlight grammar
aba2c40 
Prior to this commit, http request message code blocks in Markdown
files were not being highlighted correctly. This has been corrected by
adding the new grammar to the bundle, removing the CRLFs (`\r\n`) from
these code samples, adding a reminder to re-add them, and tuning the
syntax theme to support attribute highlighting.
mcollina
mcollina approved these changes Jun 28, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@DerekNonGeneric
Copy link
Contributor Author

Can someone please add label:"author ready"?

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 30, 2020
@Trott
Copy link
Member

Trott commented Jul 2, 2020

Landed in 1dc837e

@Trott Trott closed this Jul 2, 2020
Trott pushed a commit that referenced this pull request Jul 2, 2020
@Trott
doc: add http highlight grammar
1dc837e 
Prior to this commit, http request message code blocks in Markdown
files were not being highlighted correctly. This has been corrected by
adding the new grammar to the bundle, removing the CRLFs (`\r\n`) from
these code samples, adding a reminder to re-add them, and tuning the
syntax theme to support attribute highlighting.

PR-URL: #33785
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
@MylesBorins
doc: add http highlight grammar
726771a 
Prior to this commit, http request message code blocks in Markdown
files were not being highlighted correctly. This has been corrected by
adding the new grammar to the bundle, removing the CRLFs (`\r\n`) from
these code samples, adding a reminder to re-add them, and tuning the
syntax theme to support attribute highlighting.

PR-URL: #33785
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
@MylesBorins
doc: add http highlight grammar
af9e6f6 
Prior to this commit, http request message code blocks in Markdown
files were not being highlighted correctly. This has been corrected by
adding the new grammar to the bundle, removing the CRLFs (`\r\n`) from
these code samples, adding a reminder to re-add them, and tuning the
syntax theme to support attribute highlighting.

PR-URL: #33785
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@addaleax
doc: add http highlight grammar
e2fff1b 
Prior to this commit, http request message code blocks in Markdown
files were not being highlighted correctly. This has been corrected by
adding the new grammar to the bundle, removing the CRLFs (`\r\n`) from
these code samples, adding a reminder to re-add them, and tuning the
syntax theme to support attribute highlighting.

PR-URL: #33785
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nschonni nschonni nschonni left review comments

@rexagod rexagod rexagod approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@DerekNonGeneric @Trott @mcollina @nschonni @lpinca @rexagod @addaleax @nodejs-github-bot