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: added example for setting Vary: Accept-Encoding header in zlib.md #26308

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

mukulkhanna
Copy link
Contributor

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

Checklist

@nodejs-github-bot 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
Trott reviewed Feb 26, 2019
View reviewed changes
doc/api/zlib.md Outdated
@@ -107,6 +107,8 @@ const http = require('http');
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
Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Feb 26, 2019

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

Trott
Trott reviewed Feb 26, 2019
View reviewed changes
doc/api/zlib.md Outdated Show resolved Hide resolved
@mukulkhanna
Copy link
Contributor Author

mukulkhanna commented Feb 26, 2019 •
edited
Loading

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.

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

Trott commented Feb 26, 2019

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
Copy link
Member

Trott commented Feb 26, 2019

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

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

@mukulkhanna
Copy link
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
Copy link
Member

Trott commented Feb 26, 2019

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
Copy link
Member

richardlau commented Feb 27, 2019 •
edited
Loading

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 richardlau mentioned this pull request Feb 27, 2019
Merged
@Trott
Copy link
Member

Trott commented Feb 27, 2019

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
Copy link
Contributor Author

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

@Trott
Copy link
Member

Trott commented Feb 27, 2019

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

Leave this pull request open. Thanks!

@richardlau
Copy link
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
@mukulkhanna @Trott
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
Copy link
Member

Trott commented Feb 28, 2019

Landed in 86f13d6.

Thanks for the contribution! 🎉

@Trott Trott closed this Feb 28, 2019
@addaleax addaleax mentioned this pull request Feb 28, 2019
Closed
addaleax pushed a commit that referenced this pull request Mar 1, 2019
@mukulkhanna @addaleax
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 BridgeAR mentioned this pull request Mar 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@jasnell jasnell jasnell 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
@mukulkhanna @Trott @richardlau @jasnell @BridgeAR @nodejs-github-bot