-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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: add option to stop duplicates in history #2982
Conversation
Seems good at a glance. Perhaps it should remove the old index and replace it with the new index? Also, bash doesn't do this, so maybe this functionality would be unexpected? |
Does this mean, when I press up arrow in the REPL, it will skip the actual commands which I executed if they repeat? Is it acceptable? |
@Fishrock123 Bash does have this as an option, as does zsh, while fish has this enabled by default. I suggest Also, @Fishrock123, if a new line is identical to an old line, then the old line is removed from history and the new line is appended to the end of the history as normal. This is the behavior of bash, zsh, and fish when this option is enabled. Pardon me if I misunderstand your question. @thefourtheye No, this will be not be enabled for the |
7da4fd4
to
c7066fb
Compare
doc/api/readline.md
Outdated
@@ -358,6 +358,9 @@ added: v0.1.98 | |||
only if `terminal` is set to `true` by the user or by an internal `output` | |||
check, otherwise the history caching mechanism is not initialized at all. | |||
* `prompt` - the prompt string to use. Default: `'> '` | |||
* `historyNoDups` {boolean} If `true`, when a new input line being added to 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.
deDupeHistory
may be a better name.
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!
@DannyNemer Sorry for the extensive delay, LGTM in hindsight. |
if (this.deDupeHistory) { | ||
// Remove older history line if identical to new one | ||
const dupIndex = this.history.indexOf(this.line); | ||
if (dupIndex !== -1) this.history.splice(dupIndex, 1); |
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 would have preferred filter
over 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.
Wouldn't Array.prototype.filter()
be slower? The method would always create a new array whether or not a duplicate exists, and would traverse the entire this.history
array even after finding a match/duplicate, though there can never be more than one duplicate.
@DannyNemer One other thing, I think it should probably also do this upon loading the history, i.e. for the internal repl's persistent history from file. |
@Fishrock123 I do not think so. As is, this change does not affect Were we to add this option to (In contrast to Your thoughts? Thank you! |
Makes sense to me. |
c133999
to
83c7a88
Compare
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.
Looks mostly good to me.
@DannyNemer Sorry for taking so long to get to this, but if you could update it looks about good to go.
One other note is that this isn't useable via the REPL api unless you also pass the value at
Lines 438 to 445 in 40eba12
Interface.call(this, { | |
input: self.inputStream, | |
output: self.outputStream, | |
completer: self.completer, | |
terminal: options.terminal, | |
historySize: options.historySize, | |
prompt | |
}); |
doc/api/readline.md
Outdated
@@ -365,6 +365,9 @@ added: v0.1.98 | |||
`crlfDelay` milliseconds, both `\r` and `\n` will be treated as separate | |||
end-of-line input. Default to `100` milliseconds. | |||
`crlfDelay` will be coerced to `[100, 2000]` range. | |||
* `deDupeHistory` {boolean} If `true`, when a new input line being added to the | |||
history list duplicates an older one, removes the older line from the list. | |||
Defaults to `false`. |
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.
"input line
beingadded"
"older one, this removes the older"
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.
lib/readline.js
Outdated
@@ -48,6 +48,7 @@ function Interface(input, output, completer, terminal) { | |||
|
|||
EventEmitter.call(this); | |||
var historySize; | |||
var deDupeHistory; |
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.
Should this be initialized with false
by default?
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. I originally did not initialize the variable to match the style of the historySize
variable declaration that precedes it, but this change is immaterial.
expectedLines = ['foo', 'bar', 'baz', 'bar', 'bat', 'bat']; | ||
callCount = 0; | ||
rli.on('line', function(line) { | ||
assert.equal(line, expectedLines[callCount]); |
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 would probably be preferred if these assertions could all be their strict
variants, e.g. assert.strictEqual()
or asset.notStrictEqual()
.
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.
@DannyNemer ... is this still something you'd like to pursue? |
@Fishrock123 I implemented all suggested changes. Thank you for your help! Regarding extending the
Passing the In contrast to Your thoughts? I appreciate your suggestion. |
Adds `options.deDupeHistory` for `readline.createInterface(options)`. If `options.deDupeHistory` is `true`, when a new input line being added to the history list duplicates an older one, removes the older line from the list. Defaults to `false`. Many users would appreciate this option, as it is a common setting in shells. This option certainly should not be default behavior, as it would be problematic in applications such as the `repl`, which inherits from the readline `Interface`. Extends documentation to reflect this API addition. Adds tests for when `options.deDupeHistory` is truthy, and when `options.deDupeHistory` is falsey.
My suggestion was only for the default repl, not for any New CI: https://ci.nodejs.org/job/node-test-pull-request/6907/ |
Adds `options.deDupeHistory` for `readline.createInterface(options)`. If `options.deDupeHistory` is `true`, when a new input line being added to the history list duplicates an older one, removes the older line from the list. Defaults to `false`. Many users would appreciate this option, as it is a common setting in shells. This option certainly should not be default behavior, as it would be problematic in applications such as the `repl`, which inherits from the readline `Interface`. Extends documentation to reflect this API addition. Adds tests for when `options.deDupeHistory` is truthy, and when `options.deDupeHistory` is falsey. PR-URL: #2982 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Thanks, landed in 5bda5fa! Sorry for the huge delay on this one 😰. I've merged as-is now, if we want it to be a default option for the repl, or an env variable or something we can discuss that separately. :) |
Adds `options.deDupeHistory` for `readline.createInterface(options)`. If `options.deDupeHistory` is `true`, when a new input line being added to the history list duplicates an older one, removes the older line from the list. Defaults to `false`. Many users would appreciate this option, as it is a common setting in shells. This option certainly should not be default behavior, as it would be problematic in applications such as the `repl`, which inherits from the readline `Interface`. Extends documentation to reflect this API addition. Adds tests for when `options.deDupeHistory` is truthy, and when `options.deDupeHistory` is falsey. PR-URL: #2982 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Renames `options.deDupeHistory` → `options.removeHistoryDuplicates` for `readline.createInterface(options)`. The option name `removeHistoryDuplicates` is preferable to the semantically identical name `deDupeHistory` because "dedupe" (short for "deduplication") is obscure and neologistic while `removeHistoryDuplicates` is clear, though verbose. Updates tests and documentation for this option accordingly. PR-URL: #11950 Ref: #2982 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: * buffer: - do not segfault on out-of-range index (Timothy Gu) #11927 * crypto: - Fix memory leak if certificate is revoked (Tom Atkinson) #12089 * deps: * upgrade npm to 4.2.0 (Kat Marchán) #11389 * fix async await desugaring in V8 (Michaël Zasso) #12004 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - add native URL class (James M Snell) #11801 PR-URL: #12104
Notable changes: * buffer: - do not segfault on out-of-range index (Timothy Gu) #11927 * crypto: - Fix memory leak if certificate is revoked (Tom Atkinson) #12089 * deps: * upgrade npm to 4.2.0 (Kat Marchán) #11389 * fix async await desugaring in V8 (Michaël Zasso) #12004 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - add native URL class (James M Snell) #11801 PR-URL: #12104
Notable changes: * buffer: - do not segfault on out-of-range index (Timothy Gu) nodejs/node#11927 * crypto: - Fix memory leak if certificate is revoked (Tom Atkinson) nodejs/node#12089 * deps: * upgrade npm to 4.2.0 (Kat Marchán) nodejs/node#11389 * fix async await desugaring in V8 (Michaël Zasso) nodejs/node#12004 * readline: - add option to stop duplicates in history (Danny Nemer) nodejs/node#2982 * src: - add native URL class (James M Snell) nodejs/node#11801 PR-URL: nodejs/node#12104 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@nodejs/lts we should discuss this backport as it has been requested by @bnoordhuis |
+1 on landing in 6.x |
needs to land with #11950 |
Adds `options.deDupeHistory` for `readline.createInterface(options)`. If `options.deDupeHistory` is `true`, when a new input line being added to the history list duplicates an older one, removes the older line from the list. Defaults to `false`. Many users would appreciate this option, as it is a common setting in shells. This option certainly should not be default behavior, as it would be problematic in applications such as the `repl`, which inherits from the readline `Interface`. Extends documentation to reflect this API addition. Adds tests for when `options.deDupeHistory` is truthy, and when `options.deDupeHistory` is falsey. PR-URL: #2982 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Renames `options.deDupeHistory` → `options.removeHistoryDuplicates` for `readline.createInterface(options)`. The option name `removeHistoryDuplicates` is preferable to the semantically identical name `deDupeHistory` because "dedupe" (short for "deduplication") is obscure and neologistic while `removeHistoryDuplicates` is clear, though verbose. Updates tests and documentation for this option accordingly. PR-URL: #11950 Ref: #2982 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Adds `options.deDupeHistory` for `readline.createInterface(options)`. If `options.deDupeHistory` is `true`, when a new input line being added to the history list duplicates an older one, removes the older line from the list. Defaults to `false`. Many users would appreciate this option, as it is a common setting in shells. This option certainly should not be default behavior, as it would be problematic in applications such as the `repl`, which inherits from the readline `Interface`. Extends documentation to reflect this API addition. Adds tests for when `options.deDupeHistory` is truthy, and when `options.deDupeHistory` is falsey. PR-URL: #2982 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Renames `options.deDupeHistory` → `options.removeHistoryDuplicates` for `readline.createInterface(options)`. The option name `removeHistoryDuplicates` is preferable to the semantically identical name `deDupeHistory` because "dedupe" (short for "deduplication") is obscure and neologistic while `removeHistoryDuplicates` is clear, though verbose. Updates tests and documentation for this option accordingly. PR-URL: #11950 Ref: #2982 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) #10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - ability to select cert store at runtime (Adam Majer) #8334 - Use system CAs instead of using bundled ones (Adam Majer) #8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 - adding support for OPENSSL_CONF again (Sam Roberts) #11006 - make LazyTransform compabile with Streams1 (Matteo Collina) #12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) #11094 - upgrade libuv to 1.10.2 (cjihrig) #10717 - upgrade libuv to 1.10.1 (cjihrig) #9647 - upgrade libuv to 1.10.0 (cjihrig) #9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) #9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - support "--" after "-e" as end-of-options (John Barboza) #10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) #11005 - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 PR-URL: #13059
This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) #10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - ability to select cert store at runtime (Adam Majer) #8334 - Use system CAs instead of using bundled ones (Adam Majer) #8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 - adding support for OPENSSL_CONF again (Sam Roberts) #11006 - make LazyTransform compabile with Streams1 (Matteo Collina) #12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) #11094 - upgrade libuv to 1.10.2 (cjihrig) #10717 - upgrade libuv to 1.10.1 (cjihrig) #9647 - upgrade libuv to 1.10.0 (cjihrig) #9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) #9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - support "--" after "-e" as end-of-options (John Barboza) #10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) #11005 - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 PR-URL: #13059
Adds `options.deDupeHistory` for `readline.createInterface(options)`. If `options.deDupeHistory` is `true`, when a new input line being added to the history list duplicates an older one, removes the older line from the list. Defaults to `false`. Many users would appreciate this option, as it is a common setting in shells. This option certainly should not be default behavior, as it would be problematic in applications such as the `repl`, which inherits from the readline `Interface`. Extends documentation to reflect this API addition. Adds tests for when `options.deDupeHistory` is truthy, and when `options.deDupeHistory` is falsey. PR-URL: nodejs/node#2982 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Renames `options.deDupeHistory` → `options.removeHistoryDuplicates` for `readline.createInterface(options)`. The option name `removeHistoryDuplicates` is preferable to the semantically identical name `deDupeHistory` because "dedupe" (short for "deduplication") is obscure and neologistic while `removeHistoryDuplicates` is clear, though verbose. Updates tests and documentation for this option accordingly. PR-URL: nodejs/node#11950 Ref: nodejs/node#2982 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) nodejs/node#10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - ability to select cert store at runtime (Adam Majer) nodejs/node#8334 - Use system CAs instead of using bundled ones (Adam Majer) nodejs/node#8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) nodejs/node#9398 - adding support for OPENSSL_CONF again (Sam Roberts) nodejs/node#11006 - make LazyTransform compabile with Streams1 (Matteo Collina) nodejs/node#12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) nodejs/node#11094 - upgrade libuv to 1.10.2 (cjihrig) nodejs/node#10717 - upgrade libuv to 1.10.1 (cjihrig) nodejs/node#9647 - upgrade libuv to 1.10.0 (cjihrig) nodejs/node#9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) nodejs/node#9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) nodejs/node#10842 * readline: - add option to stop duplicates in history (Danny Nemer) nodejs/node#2982 * src: - support "--" after "-e" as end-of-options (John Barboza) nodejs/node#10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) nodejs/node#11005 - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 PR-URL: nodejs/node#13059
Checklist
Affected core subsystem(s)
readline
Description of change
Adds
options.deDupeHistory
forreadline.createInterface(options)
. Ifoptions.deDupeHistory
istrue
, when a new input line being added to the history list duplicates an older one, removes the older line from the list. Defaults tofalse
.To clarify, this option is for instantiating a new readline
Interface
; it is not for the Node REPL. The REPL inherits fromInterface
, but is unaffected (as is any existingInterface
implementation) because this option defaults tofalse
.Many users would appreciate this option for creating RLIs, as it is a common setting in shells. (This setting is availble in bash, zsh, and enabled by default for fish.)
Extends documentation to reflect this API addition.
Adds tests for when
options.deDupeHistory
is truthy, and whenoptions.deDupeHistory
is falsey.