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

Expose readline.Interface "line", "cursor", "getCursorPos" #30347

Closed
Js-Brecht opened this issue Nov 10, 2019 · 8 comments
Closed

Expose readline.Interface "line", "cursor", "getCursorPos" #30347

Js-Brecht opened this issue Nov 10, 2019 · 8 comments
Labels
readline Issues and PRs related to the built-in readline module.

Comments

@Js-Brecht
Copy link
Contributor

Describe the solution you'd like
I think it would be very useful to be able to access readline.Interface's current line and cursor values, as well as access the _getCursorPos() member function.

It's frequently desirable to draw something outside of the current prompt during input. I am endlessly recreating actions that readline does already; specifically, tracking the cursor position (so listening for left, right, home, end, delete, backspace, ctrl+, etc, etc, ad nauseam), as well as listening to stdin for data, so that I can keep track of what the input looks like. This way, if I want to draw something outside of the prompt, I know where to return the cursor to.

I also see that readline uses Interface._getCursorPos() internally; also very useful, and pretty much what I'm recreating with Interface.line & Interface.cursor calculations.

Being able to read Interface.line at any given time can be important, if you need to use that value somewhere else, but you aren't done with the prompt yet (for example, drawing an autocomplete list)

But it seems awfully redundant to have to do all of this, when readline already does it quite well. Honestly, by the time I'm done, I've basically created my own readline interface. Seems like a waste.

I can read Interface.line, Interface.cursor, etc... but they aren't exposed in @types/node, so it makes me feel like they are omitted for a reason.

Describe alternatives you've considered
There's a couple of alternatives. One, I guess, is to just use the Interface members, and hope they don't ever change. The other is essentially creating my own custom readline interface, which really is just reinventing the wheel.

Perhaps somebody could share the philosophy behind the current design? On the other hand, if they can be safely exposed in the types, I'd be happy to make a PR.
@bnoordhuis bnoordhuis added the readline Issues and PRs related to the built-in readline module. label Nov 12, 2019
@bnoordhuis
Copy link
Member

I can't speak for other collaborators but I'm okay with promoting _getCursorPos() to public API, i.e., by stripping the underscore. readline's implementation has been stable for a long time now and I don't expect that to change.

The .line and .cursor properties have simply never been documented, I think.

I suggest opening a pull request and see how it's received.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 12, 2019

I'd be OK with that suggestion as well ^

Js-Brecht added a commit to Js-Brecht/DefinitelyTyped that referenced this issue Nov 19, 2019
@Js-Brecht
Expose line/cursor properties
32ca106 
The current "line" and "cursor" states are often necessary when developing applications that use readline for prompts.

See nodejs/node#30347
@Js-Brecht Js-Brecht mentioned this issue Nov 19, 2019
Merged
9 tasks
@Js-Brecht
Copy link
Contributor Author

Opened a PR for @types/node. If I strip the underscore from _getCursorPos(), would that only make it into future releases of node? Or are patches still being released for v8+?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 20, 2019

Opened a PR for @types/node.

We should document them in this repo as well. Care to open a documentation PR?

If I strip the underscore from _getCursorPos(), would that only make it into future releases of node? Or are patches still being released for v8+?

I'd recommend stripping the underscore, but also creating an alias (_getCursorPos = getCursorPos). If you don't create the alias, it will be a breaking change and will have to wait for Node 14. If you add the alias, it can be a semver-minor change, allowing it to land in Node 13 and potentially as far back as Node 10. I think Node 8 is unlikely at this point, given that it is end-of-life next month.

@Js-Brecht
Copy link
Contributor Author

If it's all the same to you, I think I'd rather just make an alias the other way around: getCursorPos = _getCursorPos.

To me that makes more sense (even though they both effectively do the same thing); instead of changing every instance where _getCursorPos is used internally, it would just be exposing an internal member. Minimally invasive 😆.

I will make both of these PRs when I have a few more minutes to spare.

Js-Brecht added a commit to Js-Brecht/node that referenced this issue Nov 23, 2019
@Js-Brecht
doc: include line/cursor in readline documentation
7e55ab7 
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
@Js-Brecht Js-Brecht mentioned this issue Nov 26, 2019
Closed
2 tasks
Js-Brecht added a commit to Js-Brecht/DefinitelyTyped that referenced this issue Nov 26, 2019
@Js-Brecht
[node/readline] expose line/cursor properties + tests
ab9a46a 
The current "line" and "cursor" states are often necessary when developing applications that use readline for prompts.

See nodejs/node#30347
@antsmartian
Copy link
Contributor

+1 to move _getCursorPos to public API.

@Js-Brecht Js-Brecht mentioned this issue Nov 27, 2019
Closed
3 tasks
@Js-Brecht
Copy link
Contributor Author

Hey guys, sorry it's taken me so long to get these PRs opened. Had a lot going on lately.

PR #30667 is documentation for the line and cursor properties.
Just created PR #30687 for _getCursorPos() -> getCursorPos().

Js-Brecht added a commit to Js-Brecht/node that referenced this issue Dec 2, 2019
@Js-Brecht
doc: include line/cursor in readline documentation
f423395 
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
addaleax pushed a commit that referenced this issue Dec 7, 2019
@Js-Brecht @addaleax
doc: include line/cursor in readline documentation
bd9be04 
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]>
targos pushed a commit that referenced this issue Dec 9, 2019
@Js-Brecht @targos
doc: include line/cursor in readline documentation
2123d53 
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]>
Js-Brecht added a commit to Js-Brecht/node that referenced this issue Dec 9, 2019
@Js-Brecht
readline: promote _getCursorPos to public api
5f4f780 
Aliases Interface._getCursorPos to Interface.getCursorPos,
for consumption as a public API.

refs: nodejs#30347
amcasey pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Dec 9, 2019
@Js-Brecht @amcasey
[node/readline] expose line/cursor properties + tests (#40513)
141ff98 
The current "line" and "cursor" states are often necessary when developing applications that use readline for prompts.

See nodejs/node#30347
targos pushed a commit that referenced this issue Dec 16, 2019
@Js-Brecht @targos
readline: promote _getCursorPos to public api
a68729c 
Alias _getCursorPos() = getCursorPos() for backwards
compatibility.

Refs: #30347

PR-URL: #30687
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 17, 2019
@Js-Brecht @MylesBorins
readline: promote _getCursorPos to public api
5b49ded 
Alias _getCursorPos() = getCursorPos() for backwards
compatibility.

Refs: #30347

PR-URL: #30687
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@antsmartian
Copy link
Contributor

antsmartian commented Jan 7, 2020 •
edited
Loading

Thanks @Js-Brecht , this is taken care in: #30667, #30687

targos pushed a commit that referenced this issue Jan 14, 2020
@Js-Brecht @targos
doc: include line/cursor in readline documentation
0f0388c 
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]>
targos pushed a commit that referenced this issue Jan 14, 2020
@Js-Brecht @targos
readline: promote _getCursorPos to public api
95f332f 
Alias _getCursorPos() = getCursorPos() for backwards
compatibility.

Refs: #30347

PR-URL: #30687
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
@Js-Brecht @BethGriggs
doc: include line/cursor in readline documentation
6db2562 
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 issue Feb 6, 2020
@Js-Brecht @BethGriggs
readline: promote _getCursorPos to public api
7a8232a 
Alias _getCursorPos() = getCursorPos() for backwards
compatibility.

Refs: #30347

PR-URL: #30687
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bnoordhuis @antsmartian @Js-Brecht @cjihrig