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: add a line to specify fs.createWriteStream instance #33841

Closed
wants to merge 1 commit into from

Conversation

zombieleet
Copy link
Contributor

@zombieleet zombieleet commented Jun 11, 2020

Add a line to specify which fs method to call that returns an instance of fs.WriteStream.
This was specifed in the fs.ReadStream section of the fs documentation

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Add a line to specify which `fs` method to call that returns an instance of `fs.WriteStream`.
This was specifed in the fs.ReadStream section of the fs documentation
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jun 11, 2020
@mscdex
Copy link
Contributor

mscdex commented Jun 11, 2020

-1 The description for fs.createWriteStream() already indicates what type it returns.

@zombieleet
Copy link
Contributor Author

@mscdex I know about that. There is a line under fs.ReadStream that specifies that fs.createReadStream returns an instance of fs.ReadStream. This is not present under fs.WriteStream

fs.ReadStream
fs.WriteStream

@mscdex
Copy link
Contributor

mscdex commented Jun 11, 2020

If anything I'd rather remove the line from the fs.ReadStream description instead.

Comment on lines +1077 to 1079
A successful call to `fs.createWriteStream()` will return a new `fs.WriteStream` object.

* Extends {stream.Writable}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should switch the lines around here:

Suggested change
A successful call to `fs.createWriteStream()` will return a new `fs.WriteStream` object.
* Extends {stream.Writable}
* Extends {stream.Writable}
A successful call to `fs.createWriteStream()` will return a new `fs.WriteStream` object.

To match how it’s done for fs.ReadStream

@addaleax
Copy link
Member

-1 The description for fs.createWriteStream() already indicates what type it returns.

@mscdex That comment is not helpful at all, because this is not the documentation for fs.createWriteStream().

@mscdex
Copy link
Contributor

mscdex commented Jun 11, 2020

@addaleax It's relevant because it's duplicate information that is already specified in the description for fs.createWriteStream(). Users wouldn't even know to look at the documentation for fs.WriteStream to know that that's the return type of fs.createWriteStream() unless they were casually reading through the entirety of the fs documentation (and even if they were, they would eventually reach the fs.createWriteStream() description anyway).

I also see the removal of the existing wording in the documentation for fs.ReadStream as being a similar change to when we standardized the format for APIs (e.g. when we converted the fs.ReadStream description from freeform text to the * Extends: {...} format when describing its type). In this case we're removing freeform text and relying on the * Returns: {...} information.

@addaleax
Copy link
Member

@mscdex Fwiw, classes are listed in the fs table of contents before methods, so if I were to look for a way to write to a file, this would be the first entry I encounter, and at least the deprecations docs also link to methods in the class description and not the method that creates these streams.

@mscdex
Copy link
Contributor

mscdex commented Jun 11, 2020

so if I were to look for a way to write to a file, this would be the first entry I encounter

Honestly if users are coming to the API reference documentation to learn how to perform a specific task (e.g. "how to write to a file"), they are probably either going to instead:

  • Search google, which would reveal usage of fs.createWriteStream()/fs.writeFile()/etc. at which point they would then refer to the API documentation for that function.
  • CTRL-F for 'write' in the API documentation and find a sensibly-named function (fs.createWriteStream()/fs.writeFile()/etc.) that way and read the corresponding description.
  • Or look at the examples at the very top of the API documentation page (yes I am aware there is not one there specifically for fs.createWriteStream(), but that's irrelevant).

@addaleax
Copy link
Member

@mscdex If that’s the case, I would consider our docs to be insufficient. Yes, they are reference docs, but they are also all the official documentation that exists.

/cc @nodejs/documentation

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jul 3, 2020
jasnell added a commit to jasnell/node that referenced this pull request Jul 3, 2020
Alternative to nodejs#33841

Co-authored-by: zombieleet <zombieleetnca@gmail.com>
jasnell added a commit that referenced this pull request Jul 5, 2020
Alternative to #33841

Co-authored-by: zombieleet <zombieleetnca@gmail.com>

PR-URL: #34188
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jul 5, 2020

Landed alternative 6b2b886

@jasnell jasnell closed this Jul 5, 2020
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
Alternative to #33841

Co-authored-by: zombieleet <zombieleetnca@gmail.com>

PR-URL: #34188
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
Alternative to #33841

Co-authored-by: zombieleet <zombieleetnca@gmail.com>

PR-URL: #34188
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 27, 2020
Alternative to #33841

Co-authored-by: zombieleet <zombieleetnca@gmail.com>

PR-URL: #34188
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 28, 2020
Alternative to #33841

Co-authored-by: zombieleet <zombieleetnca@gmail.com>

PR-URL: #34188
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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. fs Issues and PRs related to the fs subsystem / file system. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants