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 spelling of API name in 10.0.0 changelog #20257

Closed

Conversation

tniessen
Copy link
Member

This is a trivial change (convertNONProtocolsconvertNPNProtocols).

Refs: #19403

Checklist

@tniessen tniessen added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 24, 2018
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 24, 2018
@devsnek
Copy link
Member

devsnek commented Apr 24, 2018

as long as there's already a PR open, can you also change fs.promises to fs/promises?

@jasnell
Copy link
Member

jasnell commented Apr 24, 2018

DOH! sigh ...lol

@tniessen
Copy link
Member Author

@devsnek Is this correct?

@Trott
Copy link
Member

Trott commented Apr 24, 2018

+1 to fast-tracking

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM +1 to fast track

@Trott
Copy link
Member

Trott commented Apr 24, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2018
@tniessen
Copy link
Member Author

Landed in ad5307f.

@tniessen tniessen closed this Apr 24, 2018
tniessen added a commit that referenced this pull request Apr 24, 2018
PR-URL: #20257
Refs: #19403
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@@ -58,7 +58,7 @@
* EventEmitter
* The `EventEmitter.prototype.off()` method has been added as an alias for `EventEmitter.prototype.removeListener()`. [[`3bb6f07d52`](https://github.com/nodejs/node/commit/3bb6f07d52)]
* File System
* The `fs.promises` API provides experimental promisified versions of the `fs` functions. [[`329fc78e49`](https://github.com/nodejs/node/commit/329fc78e49)]
* The `fs/promises` API provides experimental promisified versions of the `fs` functions. [[`329fc78e49`](https://github.com/nodejs/node/commit/329fc78e49)]
Copy link
Member

Choose a reason for hiding this comment

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

This change is actually wrong. Originally it did indeed land as fs.promises. I would have kept it as is therefore. There should be another commit that changed it later on.

Copy link
Member

Choose a reason for hiding this comment

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

A good alternative would be to add the commit that changed it to fs/promises.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to do either, I simply relied on @devsnek's and others reviews.

Copy link
Member

Choose a reason for hiding this comment

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

But this isn't a commit message. This is a textual list of notable changes. Unless v10.0.0 shipped with fs.promises rather than fs/promises, this change is correct, IMO. Later on, when the commits are all listed, it says fs.promises and that is correct for the reason you identify here. But this section should not be true to specific commits but instead explain the notable change in the release.

Copy link
Member

Choose a reason for hiding this comment

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

This does raise an interesting question though, which is:

Is it necessary to list the commits with the notable changes?

Perhaps it is helpful in some ways, but perhaps it is also misleading. A notable change will often receive subsequent modifications after initially landing. We are not doing the end user favors by providing information in "Notable changes" that describes a change in a form that it was never actually released.

Might this entry in particular be better with no link to a commit and maybe a link to relevant docs about the feature instead?

People reading release notes rarely want to look at source changes, I imagine. Are we making the mistake of designing these release notes for us and not for the end user? Do we need Release Notes that are separate from the ChangeLog and the ChangeLog should be more dry listings and less text? (I don't know the answer here, and this is probably not the place to discuss it, but if someone wants to discuss this in the Release repo or discussion board, maybe that's good?)

@BridgeAR
Copy link
Member

I would like to rebase this commit out and remove the change that I pointed out.

@Trott
Copy link
Member

Trott commented Apr 24, 2018

I would like to rebase this commit out and remove the change that I pointed out.

I believe the change is correct as is, as explained in #20257 (comment).

MylesBorins pushed a commit that referenced this pull request May 4, 2018
PR-URL: #20257
Refs: #19403
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants