GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: simplify readline/stdin text by Trott · Pull Request #41583 · nodejs/node

Conversation

@Trott

@Trott Trott commented Jan 18, 2022

Copy link
Copy Markdown
Member

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@Trott
doc: simplify readline/stdin text
ddb9e81
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. readline Issues and PRs related to the built-in readline module. labels Jan 18, 2022
Mesteery
Mesteery approved these changes Jan 18, 2022
View reviewed changes
tniessen
tniessen reviewed Jan 18, 2022
View reviewed changes
Comment thread doc/api/readline.md
process.stdin.unref();
```
will not terminate until it receives an [EOF character][]. To exit without
waiting for user input, call `process.stdin.unref()`.

@tniessen tniessen Jan 18, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Trott Trott Jan 19, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Trott

Trott commented Jan 22, 2022

Copy link
Copy Markdown
Member Author

@nodejs/documentation

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax
addaleax approved these changes Jan 22, 2022
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Trott Trott added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 23, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 23, 2022
@nodejs-github-bot
nodejs-github-bot merged commit fbfb61a into nodejs:master Jan 23, 2022
@nodejs-github-bot

nodejs-github-bot commented Jan 23, 2022

Copy link
Copy Markdown
Collaborator

Landed in fbfb61a

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
@Trott @BethGriggs
doc: simplify readline/stdin text
0d6227c
PR-URL: #41583
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Feb 8, 2022
v17.5.0 release proposal #41897
Merged
danielleadams pushed a commit that referenced this pull request Feb 28, 2022
@Trott @danielleadams
doc: simplify readline/stdin text
8c6fa34
PR-URL: #41583
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
@Trott @danielleadams
doc: simplify readline/stdin text
dbf6486
PR-URL: #41583
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@danielleadams danielleadams mentioned this pull request Mar 3, 2022
v16.14.1 release proposal #42200
Merged
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
@Trott @danielleadams
doc: simplify readline/stdin text
ee19431
PR-URL: #41583
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
@Trott @danielleadams
doc: simplify readline/stdin text
d5efecd
PR-URL: #41583
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen left review comments
@addaleax addaleax addaleax approved these changes
+1 more reviewer
@Mesteery Mesteery Mesteery approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. readline Issues and PRs related to the built-in readline module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@Trott @nodejs-github-bot @addaleax @tniessen @Mesteery
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.