FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: added example for setting Vary: Accept-Encoding header in zlib.md by mukulkhanna · Pull Request #26308 · nodejs/node · GitHub

/ node Public

doc: added example for setting Vary: Accept-Encoding header in zlib.md - #26308

Closed
mukulkhanna wants to merge 3 commits into
nodejs:masterfrom
mukulkhanna:25495
ConversationCommits (3)ChecksFiles changed
Closed

doc: added example for setting Vary: Accept-Encoding header in zlib.md#26308
mukulkhanna wants to merge 3 commits into
nodejs:masterfrom
mukulkhanna:25495

Conversation

mukulkhanna commented Feb 26, 2019

Copy link
Copy Markdown
Contributor

Resolving issue #25495.
Added example to set Vary: Accept-Encoding header with explanatory comment to doc/api/zlib.md

Checklist

nodejs-github-bot added doc Issues and PRs related to the documentations. zlib Issues and PRs related to the zlib subsystem. labels Feb 26, 2019
Trott reviewed Feb 26, 2019
View reviewed changes
Comment thread doc/api/zlib.md Outdated
const fs = require('fs');
http.createServer((request, response) => {
const raw = fs.createReadStream('index.html');
// instruct the proxy to store both a compressed and uncompressed version of the resource

Trott Feb 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi, @mukulkhanna! Welcome and thanks for the pull request! This line above is (probably) going to cause the linter to complain. Could you wrap the comment at 80 characters and capitalize the first letter of the first word? (Might as well put a period/full-stop at the end while you're at it.)

Trott Feb 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh, and you can check the linting with make lint (or vcbuild lint on Windows).

mukulkhanna Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi @Trott.
Sure, will do asap, thank you.

Trott commented Feb 26, 2019

Copy link
Copy Markdown
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2708/

Trott reviewed Feb 26, 2019
View reviewed changes
Comment thread doc/api/zlib.md Outdated

mukulkhanna commented Feb 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2708/

@Trott ,
I don't have overall/read access to the link.

Fixes: nodejs#25495
2b7f394 
Co-Authored-By: mukulkhanna <mukul18khanna@gmail.com>

Trott commented Feb 26, 2019

Copy link
Copy Markdown
Member

I don't have overall/read access to the link.

Correct. CI is currently locked down while it is being used to prepare the security release that is scheduled to come out in the next several hours.

Trott commented Feb 26, 2019

Copy link
Copy Markdown
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2709/

(Results should be reported in the GitHub widget below.)

mukulkhanna commented Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

The 'first' commit message is failing the test.

Is this the first commit of this pull request or is this about the most recent commit in the pull request.

Trott commented Feb 26, 2019

Copy link
Copy Markdown
Member

The 'first' commit message is failing the test.

Is this the first commit of this pull request or is this about the most recent commit in the pull request.

It's the first word of the last commit, but don't worry about it. Whoever lands this commit will need to squash the commits into one and modify the commit message to conform to our requirements anyway.

The Travis stuff is advisory, but the other two checks from Jenkins are green and those are the ones that matter.

richardlau commented Feb 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

The 'first' commit message is failing the test.
Is this the first commit of this pull request or is this about the most recent commit in the pull request.

It's the first word of the last commit, but don't worry about it. Whoever lands this commit will need to squash the commits into one and modify the commit message to conform to our requirements anyway.

The Travis stuff is advisory, but the other two checks from Jenkins are green and those are the ones that matter.

Actually the Travis failure is the first commit: https://travis-ci.com/nodejs/node/jobs/180651661#L476-L486

*** Commit message for d510b68c0d is:
doc: added example for setting Vary: Accept-Encoding header in zlib.md
+npx -q core-validate-commit --no-validate-metadata d510b68c0d167d26964da3a8d3aa37df4ecb2866
  ✖  d510b68c0d167d26964da3a8d3aa37df4ecb2866
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✖  0:38     First word after subsystem(s) in title should be lowercase. title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length

In this case core-validate-commit is incorrectly treating Accept-Encoding as a first word after subsystem(s) as it comes after a second :. I'll raise a PR to fix core-validate-commit. (Edit: nodejs/core-validate-commit#48)

richardlau mentioned this pull request Feb 27, 2019
Merged

Trott commented Feb 27, 2019

Copy link
Copy Markdown
Member

Actually the Travis failure is the first commit: https://travis-ci.com/nodejs/node/jobs/180651661#L476-L486

@richardlau Ooh, I didn't even consider that it's a bug. (And I'm the one that initially set up Travis to always lint the first commit and no other commit! You'd think I'd remember that. 😆) Nice catch! Sorry for the bad information!

mukulkhanna commented Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

@Trott Can I close this pull request or should I wait for it to be merged ?

Trott commented Feb 27, 2019

Copy link
Copy Markdown
Member

@Trott Can I close this pull request or should I wait for it to be merged ?

Leave this pull request open. Thanks!

richardlau commented Feb 28, 2019

Copy link
Copy Markdown
Member

Actually the Travis failure is the first commit: https://travis-ci.com/nodejs/node/jobs/180651661#L476-L486

@richardlau Ooh, I didn't even consider that it's a bug. (And I'm the one that initially set up Travis to always lint the first commit and no other commit! You'd think I'd remember that. 😆) Nice catch! Sorry for the bad information!

FTR I've pushed out a fix to core-validate-commit and reran the failing the Travis job (which now passes 🎉).

Trott pushed a commit to Trott/io.js that referenced this pull request Feb 28, 2019
doc: add example for setting Vary: Accept-Encoding header in zlib.md
86f13d6 
PR-URL: nodejs#26308
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Trott commented Feb 28, 2019

Copy link
Copy Markdown
Member

Landed in 86f13d6.

Thanks for the contribution! 🎉

Trott closed this Feb 28, 2019
addaleax mentioned this pull request Feb 28, 2019
Closed
addaleax pushed a commit that referenced this pull request Mar 1, 2019
doc: add example for setting Vary: Accept-Encoding header in zlib.md
40a5a93 
PR-URL: #26308
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR mentioned this pull request Mar 4, 2019
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
jasnell jasnell approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. zlib Issues and PRs related to the zlib subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL