-
Notifications
You must be signed in to change notification settings - Fork 30k
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: document readline keybindings #20825
doc: document readline keybindings #20825
Conversation
doc/api/tty.md
Outdated
@@ -158,7 +158,109 @@ The `tty.isatty()` method returns `true` if the given `fd` is associated with | |||
a TTY and `false` if it is not, including whenever `fd` is not a non-negative | |||
integer. | |||
|
|||
## TTY keybindings |
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.
Can we document these in the readline doc? While a TTY is required for most of these, they shouldonly affect readline behaviour
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.
I guess it would be fine to add a reference to those in the repl
and in here.
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.
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.
@shobhitchittora Yes, that’s the idea (although I don’t really understand why this would be added to the TTY
doc)
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.
@addaleax I saw this code in readline -
Interface.prototype.write = function(d, key) {
if (this.paused) this.resume();
this.terminal ? this._ttyWrite(d, key) : this._normalWrite(d);
};
and thought that TTY case need a doc. But yeah we can have the table in readline
doc and ref it in REPL
and / or TTY
.
Need you opinion on this.
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.
Yeah, exactly – it’s code in readline. :) We should point out that these codes apply only for TTYs, but it’s not something that’s interesting for code that uses TTYs in general.
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.
@addaleax you are right, the TTY docs are actually the wrong place for this. The repl
should have a reference however.
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.
Done.
Nit: |
doc/api/readline.md
Outdated
@@ -287,7 +287,8 @@ added: v0.1.98 | |||
|
|||
The `rl.write()` method will write either `data` or a key sequence identified | |||
by `key` to the `output`. The `key` argument is supported only if `output` is | |||
a [TTY][] text terminal. | |||
a [TTY][] text terminal. See [`TTY keybindings`][] for a list of covered key |
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.
Nit: remove "covered"?
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.
Done.
1. Adds ref in repl doc 2. Removes ref from tty doc Closes: nodejs#20814
doc/api/repl.md
Outdated
@@ -634,3 +636,4 @@ For an example of running a REPL instance over [curl(1)][], see: | |||
[`util.inspect()`]: util.html#util_util_inspect_object_options | |||
[curl(1)]: https://curl.haxx.se/docs/manpage.html | |||
[stream]: stream.html | |||
[`TTY keybindings`]: readline.html#readline_tty_keybindings |
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.
It seems we do not need backticks in this case (this is not a code fragment). So this should be [TTY keybindings]
and go before the [curl(1)]:
(as per ASCII sorting we use in reference lists).
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.
done.
doc/api/tty.md
Outdated
@@ -161,4 +161,4 @@ integer. | |||
[`net.Socket`]: net.html#net_class_net_socket | |||
[`process.stdin`]: process.html#process_process_stdin | |||
[`process.stdout`]: process.html#process_process_stdout | |||
[`process.stderr`]: process.html#process_process_stderr | |||
[`process.stderr`]: process.html#process_process_stderr |
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.
Missing line break at the end of the file?
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.
done
doc/api/repl.md
Outdated
@@ -67,6 +67,8 @@ The following key combinations in the REPL have these special effects: | |||
variables. When pressed while entering other input, displays relevant | |||
autocompletion options. | |||
|
|||
For a full list of special keys, refer [`TTY keybindings`][]. |
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.
refer
-> refer to
?
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.
done
doc/api/readline.md
Outdated
@@ -535,3 +637,4 @@ rl.on('line', (line) => { | |||
[TTY]: tty.html | |||
[Writable]: stream.html#stream_writable_streams | |||
[reading files]: #readline_example_read_file_stream_line_by_line | |||
[`TTY keybindings`]: #readline_tty_keybindings |
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.
It seems we do not need backticks in this case (this is not a code fragment). So this should be [TTY keybindings]
and go after the [TTY]:
(as per ASCII sorting we use in reference lists).
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.
done
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.
We need the same fix in the text)
doc/api/readline.md
Outdated
</tr> | ||
<tr> | ||
<td><code>ctrl+c</code></td> | ||
<td>emits SIGINT</td> |
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.
emits
-> emit
for consistency with other cells?
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.
done
doc/api/readline.md
Outdated
</tr> | ||
<tr> | ||
<td><code>ctrl+a</code></td> | ||
<td>goto start of line</td> |
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.
goto
-> go to
?
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.
done
doc/api/readline.md
Outdated
</tr> | ||
<tr> | ||
<td><code>ctrl+e</code></td> | ||
<td>goto to end of line</td> |
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.
goto to
-> go to
?
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.
done
doc/api/readline.md
Outdated
</tr> | ||
<tr> | ||
<td><code>ctrl+p</code></td> | ||
<td>prev history item </td> |
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.
prev
-> previous
for more formal style?
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.
done.
Should we note that some bindings are OS-dependent? For example, these seem to not work for me on Windows as documented (tested in REPL): |
1. typo fix 2. adding new lines Closes: nodejs#20814
@vsemozhetbyt I think it'd be great if we can do that. Unfortunately I don't have a windows system and cannot verify bindings for the same. All help is appreciated. 😇 |
doc/api/repl.md
Outdated
[curl(1)]: https://curl.haxx.se/docs/manpage.html | ||
[stream]: stream.html | ||
|
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.
This empty line seems unneeded)
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.
done.
cc @nodejs/platform-windows: can we have some more verification of keybindings and suggestions how to better document possible differences (if we should)? |
The |
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 with the whitespace removed. My two comments would also be nice but I think that could also be done later on (even though I would prefer to have it that way right away).
doc/api/readline.md
Outdated
</tr> | ||
<tr> | ||
<td><code>ctrl+shift+backspace</code></td> | ||
<td>delete line left</td> |
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.
Not a must but I personally would prefer the descriptions all to start with a upper case character.
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.
done.
doc/api/readline.md
Outdated
<th>Description</th> | ||
</tr> | ||
<tr> | ||
<td><code>ctrl+shift+backspace</code></td> |
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.
Suggestion: add whitespace between the individual commands and maybe also only highlight the keys, not the plus. As in: <code>ctrl</code> + <code>shift</code> + <code>backspace</code>
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.
Any reason these are wrapped in <code>
and not <kbd>
?
Hm..
|
doc/api/readline.md
Outdated
</tr> | ||
<tr> | ||
<td><code>ctrl</code> + <code>z</code></td> | ||
<td>(need clarification)</td> |
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.
Hey team.
Need clarification on this keybinding. I cannot think of a description for this. Can someone help with this?
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.
I do not know, sorry(
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.
@BridgeAR I found this for ctrl
+ z
-
https://nodejs.org/api/readline.html#readline_event_sigtstp
If it's fine we can either ref to this or take the first paragraph and add here.
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.
Please document it as e.g.: Moves running process into background. Type 'fg' and press 'enter' to return.
.
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.
done.
We have a linter issue: 06:00:06 Running Markdown linter on docs...
06:00:51 doc/api/readline.md
06:00:51 596:113 warning Line must be at most 80 characters maximum-line-length remark-lint
06:00:51 599:108 warning Line must be at most 80 characters maximum-line-length remark-lint
06:00:51 623:105 warning Line must be at most 80 characters maximum-line-length remark-lint |
doc/api/readline.md
Outdated
<td>Moves running process into background. Type <code>fg</code> and press <code>enter</code> to return.</td> | ||
</tr> | ||
<tr> | ||
<td><code>ctrl</code> + <code>w</code> or <code>ctrl</code> + <code>backspace</code></td> |
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.
Are these
intended here and below?
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.
Yeah! added them for more spacing, but removing for now.
Should we land as is or need we elaborate some note about OS-dependent differences? |
I think we should document differences. And double check, since it looks like |
Let's do not stall this PR. |
@vsemozhetbyt please do let know if anything update / commit is required from my side. |
@nodejs/platform-macos Are there any additions from your team? |
ping @nodejs/documentation |
@vsemozhetbyt any updates on this? |
Sorry, it seems I've exhausted all the ways I could help( |
</tr> | ||
<tr> | ||
<td><code>ctrl</code> + <code>left</code></td> | ||
<td>Word left </td> |
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.
On macOS, it's command+option+left. ctrl+left does not work.
</tr> | ||
<tr> | ||
<td><code>ctrl</code> + <code>right</code></td> | ||
<td>Word right</td> |
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.
On macOS, it's command+option+right. ctrl+right does not work.
</tr> | ||
<tr> | ||
<td><code>meta</code> + <code>backspace</code></td> | ||
<td>Delete word left </td> |
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.
Does not work on macOS.
@shobhitchittora @vsemozhetbyt I've tested on my macOS laptop and have put the result in above as best I can. ("Meta" on macOS is "hold down the ESC key" and "backspace" is "fn + delete", in case that's relevant.) Can that information be incorporated and this unstalled? Does anything else need to happen to unstall this? |
Optimistically removing the |
@shobhitchittora Are you planning to finish this one? Or would it make sense to put a |
Closing this due to inactivity. Please reopen if needed. |
This is a rework of closed pr nodejs#20825 fixes: nodejs#20814
This documents all readline key bindings. It is a rework of #20825 PR-URL: #31256 Fixes: #20814 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
This documents all readline key bindings. It is a rework of #20825 PR-URL: #31256 Fixes: #20814 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
This documents all readline key bindings. It is a rework of #20825 PR-URL: #31256 Fixes: #20814 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
This documents all readline key bindings. It is a rework of nodejs#20825 PR-URL: nodejs#31256 Fixes: nodejs#20814 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
This documents all readline key bindings. It is a rework of #20825 PR-URL: #31256 Fixes: #20814 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Closes: #20814
make -j4 test
(UNIX), orvcbuild test
(Windows) passestests and/or benchmarks are includedAffected subsystem(s)
doc