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

[readline doc] include line/cursor in readline documentation #30667

Closed

Conversation

Js-Brecht
Copy link
Contributor

Documents the existence and purpose of the line and cursor properties, in an effort to make their usage public.

line can be used for reading the current input value during runtime, if reading from a TTY stream. Both properties are necessary when developing a custom CLI input process using readline as an input processor. This allows you to utilize the work that's been done in readline, instead of trying to recreate it.

Refs: #30347
Refs: DefinitelyTyped/DefinitelyTyped#40513

Checklist

Documents the existence and purpose of the `line` and `cursor`
properties.  `line` can be used for reading the current input value
during runtime, if reading from a TTY stream.  Both properties are
necessary when developing a custom CLI input process using `readline`
as a backend.

Refs: nodejs#30347
Refs: DefinitelyTyped/DefinitelyTyped#40513
@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 Nov 26, 2019
@addaleax
Copy link
Member

@nodejs/readline

@Js-Brecht
Copy link
Contributor Author

@addaleax Is it normal for it to take this long to get a review? Not trying to rush you guys at all; I'm sure that you all have tremendous work loads. Just wondering so I know what to expect 🙂

@addaleax
Copy link
Member

addaleax commented Dec 6, 2019

@Js-Brecht I mean, it depends, but pings are generally appreciated.

I’m not sure I can really review this because it’s not obvious to me whether this should be public, documented API or not; in any case, added: REPLACEME seems a bit odd because this being a docs-only PR usually means that the properties already exist and have been added in earlier releases?

@Js-Brecht
Copy link
Contributor Author

Js-Brecht commented Dec 6, 2019

@addaleax not sure who to ping here; I see you pinged the readline team already, so I suppose I can just wait for them.

in any case, added: REPLACEME seems a bit odd because this being a docs-only PR usually means that the properties already exist and have been added in earlier releases?

That was my initial instinct, as well, until I saw this portion of the contributing guide:

If you are adding to or deprecating an API, use REPLACEME for the version number in the documentation YAML.

I wasn't entirely sure if the practice was to use the version when the documentation was added, or when the property itself was added. Regardless, the standard syntax for the rest of the docs seem to include the YAML documentation, so I felt I would be remiss by not including it.

If the version number here refers to when the documentation was added, then REPLACEME seems appropriate. If not, do you think I should try to find the version where these particular properties were added, or simply exclude the YAML doc altogether?

I'd imagine the line property will have been around since the beginning of the Interface class (and most likely the cursor property, as well); perhaps v0.1.104? The version numbers for the members of that class do not seem to all seem to agree, which would make sense if those versions were later the class's. Case in point: several of the events were added in v0.1.98 (before the class itself?), including the line event. This is why the meaning/purpose of that version number, or that portion of the guide, is not entirely clear to me.

I could just go with the lowest common denominator, but before I did that, I would need to go digging to see if I can find the first appearance 🤔.

@addaleax
Copy link
Member

addaleax commented Dec 6, 2019

If not, do you think I should try to find the version where these particular properties were added, or simply exclude the YAML doc altogether?

Yes, that would be the right thing here – it usually doesn’t matter because newly added documentation is also for newly added code :)

I'd imagine the line property will have been around since the beginning of the Interface class (and most likely the cursor property, as well); perhaps v0.1.104? The version numbers for the members of that class do not seem to all seem to agree, which would make sense if those versions were later the class's. Case in point: several of the events were added in v0.1.98 (before the class itself?), including the line event. This is why the meaning/purpose of that version number, or that portion of the guide, is not entirely clear to me.

It’s v0.1.98 here. The reason for the class itself being marked with v0.1.104 is that that was when the class export was added; i.e. readline.createInterface() is older than readline.Interface.

doc/api/readline.md Outdated Show resolved Hide resolved
Js-Brecht and others added 3 commits December 6, 2019 13:53
Verified property exists at the creation of the readline module: version 0.1.98
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 6, 2019
addaleax pushed a commit that referenced this pull request Dec 7, 2019
Documents the existence and purpose of the `line` and `cursor`
properties.  `line` can be used for reading the current input value
during runtime, if reading from a TTY stream.  Both properties are
necessary when developing a custom CLI input process using `readline`
as a backend.

Refs: #30347
Refs: DefinitelyTyped/DefinitelyTyped#40513

PR-URL: #30667
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax
Copy link
Member

addaleax commented Dec 7, 2019

Landed in bd9be04

@addaleax addaleax closed this Dec 7, 2019
targos pushed a commit that referenced this pull request Dec 9, 2019
Documents the existence and purpose of the `line` and `cursor`
properties.  `line` can be used for reading the current input value
during runtime, if reading from a TTY stream.  Both properties are
necessary when developing a custom CLI input process using `readline`
as a backend.

Refs: #30347
Refs: DefinitelyTyped/DefinitelyTyped#40513

PR-URL: #30667
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
Documents the existence and purpose of the `line` and `cursor`
properties.  `line` can be used for reading the current input value
during runtime, if reading from a TTY stream.  Both properties are
necessary when developing a custom CLI input process using `readline`
as a backend.

Refs: #30347
Refs: DefinitelyTyped/DefinitelyTyped#40513

PR-URL: #30667
Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Documents the existence and purpose of the `line` and `cursor`
properties.  `line` can be used for reading the current input value
during runtime, if reading from a TTY stream.  Both properties are
necessary when developing a custom CLI input process using `readline`
as a backend.

Refs: #30347
Refs: DefinitelyTyped/DefinitelyTyped#40513

PR-URL: #30667
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

3 participants