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

Closed
wants to merge 2 commits into from
Closed

doc: fix spelling of API name in 10.0.0 changelog #20257

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dd70a3
doc: fix spelling of API name in 10.0.0 changelog
tniessen Apr 24, 2018
789146b
fixup! doc: fix spelling of API name in 10.0.0 changelog
tniessen Apr 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/changelogs/CHANGELOG_V10.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?)

* Invalid path errors are now thrown synchronously. [[`d8f73385e2`](https://github.com/nodejs/node/commit/d8f73385e2)]
* The `fs.readFile()` method now partitions reads to avoid thread pool exhaustion. [[`67a4ce1c6e`](https://github.com/nodejs/node/commit/67a4ce1c6e)]
* HTTP
Expand Down Expand Up @@ -88,7 +88,7 @@
* Timers
* The `enroll()` and `unenroll()` methods have been deprecated. [[`68783ae0b8`](https://github.com/nodejs/node/commit/68783ae0b8)]
* TLS
* The `tls.convertNONProtocols()` method has been deprecated. [[`9204a0db6e`](https://github.com/nodejs/node/commit/9204a0db6e)]
* The `tls.convertNPNProtocols()` method has been deprecated. [[`9204a0db6e`](https://github.com/nodejs/node/commit/9204a0db6e)]
* Support for NPN (next protocol negotiation) has been dropped. [[`5bfbe5ceae`](https://github.com/nodejs/node/commit/5bfbe5ceae)]
* The `ecdhCurve` default is now `'auto'`. [[`af78840b19`](https://github.com/nodejs/node/commit/af78840b19)]
* Trace Events
Expand Down