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

doc: fix added: info for some methods #42661

Merged
merged 1 commit into from Apr 11, 2022

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Apr 8, 2022

outgoingMessage.getHeader(), outgoingMessage.getHeaderNames(), and
outgoingMessage.getHeaders() were added to Node.js v7.7.0 via
3e8d43d.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Apr 8, 2022
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM

doc/api/http.md Outdated
Comment on lines 1662 to 1664
added:
- v8.0.0
- v7.7.0
Copy link
Contributor

@aduh95 aduh95 Apr 8, 2022

Choose a reason for hiding this comment

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

So this feature landed on v7.7.0? Why do we want to add v8.0.0 on this list? I don't think that's an interesting addition, all features of v7.7.0 are on v8.0.0 (or we should list all releases that happen since v7.7.0).

Suggested change
added:
- v8.0.0
- v7.7.0
added: v7.7.0

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the methods were added to v8.0.0 and backported to v7.7.0. See PR description and commit message.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do the same for other backports.

Copy link
Member Author

@lpinca lpinca Apr 9, 2022

Choose a reason for hiding this comment

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

Do you mean that since there are no "holes" it does not make sense to list v8.0.0? In that case I see your point but the info is a bit misleading.

Copy link
Member Author

@lpinca lpinca Apr 9, 2022

Choose a reason for hiding this comment

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

There are other occurrences like this that we might want to fix if you have a strong opinion about this:

Personally, I think this is more correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Node.js 8.0.0 was released on 2017-05-30, Node.js 7.7.0 was released 3 months before that on 2017-02-28. I don't see how it's relevant to our user if the feature landed on the Current release line because of a backport or as a regular cherry-picking, the backport doesn't make it special IMO.

That's different, Node.js v6.13.0 was a LTS release, so it makes sense to list it there as it first landed on v7.0.0.

That's an interesting one, v4.2.0 is listed as an LTS release (with the --check change)

## 2015-10-07, Version 4.2.0 'Argon' (LTS), @jasnell
### Notable changes
The first Node.js LTS release! See <https://github.com/nodejs/LTS/> for details of the LTS process.
* **icu**: Updated to version 56 with significant performance improvements (Steven R. Loomis) [#3281](https://github.com/nodejs/node/pull/3281)
* **node**:
* Added new `-c` (or `--check`) command line argument for checking script syntax without executing the code (Dave Eddy) [#2411](https://github.com/nodejs/node/pull/2411)

So one could think that v5.0.0 was the Current release that landed first, but it looks like it was released after v4.2.0, and --check change is not listed on its changelog:

## 2015-10-29, Version 5.0.0 (Stable), @rvagg

So yes, I think we should fix that.

doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
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

`outgoingMessage.getHeader()`, `outgoingMessage.getHeaderNames()`, and
`outgoingMessage.getHeaders()` were added to Node.js v7.7.0 via
3e8d43d.
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2022
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

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 11, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 11, 2022
@nodejs-github-bot nodejs-github-bot merged commit 4508c8c into nodejs:master Apr 11, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 4508c8c

@lpinca lpinca deleted the fix/added-info branch April 11, 2022 05:22
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
`outgoingMessage.getHeader()`, `outgoingMessage.getHeaderNames()`, and
`outgoingMessage.getHeaders()` were added to Node.js v7.7.0 via
3e8d43d.

PR-URL: nodejs#42661
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Qingyu Deng <i@ayase-lab.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
`outgoingMessage.getHeader()`, `outgoingMessage.getHeaderNames()`, and
`outgoingMessage.getHeaders()` were added to Node.js v7.7.0 via
3e8d43d.

PR-URL: #42661
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Qingyu Deng <i@ayase-lab.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
`outgoingMessage.getHeader()`, `outgoingMessage.getHeaderNames()`, and
`outgoingMessage.getHeaders()` were added to Node.js v7.7.0 via
3e8d43d.

PR-URL: #42661
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Qingyu Deng <i@ayase-lab.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
`outgoingMessage.getHeader()`, `outgoingMessage.getHeaderNames()`, and
`outgoingMessage.getHeaders()` were added to Node.js v7.7.0 via
3e8d43d.

PR-URL: #42661
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Qingyu Deng <i@ayase-lab.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
`outgoingMessage.getHeader()`, `outgoingMessage.getHeaderNames()`, and
`outgoingMessage.getHeaders()` were added to Node.js v7.7.0 via
3e8d43d.

PR-URL: #42661
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Qingyu Deng <i@ayase-lab.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
`outgoingMessage.getHeader()`, `outgoingMessage.getHeaderNames()`, and
`outgoingMessage.getHeaders()` were added to Node.js v7.7.0 via
3e8d43d165ef.

PR-URL: nodejs/node#42661
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Qingyu Deng <i@ayase-lab.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants