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: simplify readline/stdin text #41583

Merged
merged 1 commit into from
Jan 23, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 18, 2022

No description provided.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. labels Jan 18, 2022
process.stdin.unref();
```
will not terminate until it receives an [EOF character][]. To exit without
waiting for user input, call `process.stdin.unref()`.
Copy link
Member

Choose a reason for hiding this comment

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

Is removing the reference to the unref() documentation intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes although it wasn't an easy decision. The choices were:

  • Use wording that isn't straightforward simply so we can include unref() by itself and link that to the net docs. (This is what the existing text does.)
  • Use the simple/straightforward wording here but add a possibly-confusing link for process.stdin.unref() to the net doc description of unref().
  • Use the simple/straightforward wording here that tells the user what they need to know, and don't bother with a link to unref(). At a later date, address the fact that our docs can be non-intuitive. If I find process.stdin.unref() mentioned in code or a tutorial, good luck finding it in the docs. I'm not sure what the solution is, but I think we should solve it. We should not be relying on linking every mention to a surprising place.

@Trott
Copy link
Member Author

Trott commented Jan 22, 2022

@nodejs/documentation

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd also be good with removing this paragraph entirely, since it's not really related to readline per se and instead a general property of TTYs

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 23, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 23, 2022
@nodejs-github-bot nodejs-github-bot merged commit fbfb61a into nodejs:master Jan 23, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in fbfb61a

@Trott Trott deleted the readline-you-can branch January 23, 2022 03:21
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
PR-URL: #41583
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41583
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Feb 28, 2022
PR-URL: #41583
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
PR-URL: #41583
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
PR-URL: #41583
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
PR-URL: #41583
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants