repl: Auto alignment for .editor mode #8241

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@princejwesley
Contributor

princejwesley commented Aug 23, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change

When in .editor mode, current line whitespace prefixes
are preserved in the subsequent line.

node ๐Ÿ™ˆ โ‚น node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!'); //On enter, cursor will be at 2nd position
  _
repl: Auto alignment for .editor mode
When in `.editor` mode, current line whitespace prefixes
are preserved in the subsequent line.

```js
node ๐Ÿ™ˆ โ‚น node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!'); //On enter, cursor will be at 2 position
  _
```
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 23, 2016

Member

Nice. Nothing here stands out as an issue, LGTM with green CI!

Member

jasnell commented Aug 23, 2016

Nice. Nothing here stands out as an issue, LGTM with green CI!

@princejwesley

This comment has been minimized.

Show comment
Hide comment
lib/readline.js
@@ -247,6 +248,10 @@ Interface.prototype._addHistory = function() {
// if the history is disabled then return the line
if (this.historySize === 0) return this.line;
+ const line = this.line.replace(/^\s+|\s+$/, '');

This comment has been minimized.

@addaleax

addaleax Aug 24, 2016

Member

Why not just this.line.trim()?

@addaleax

addaleax Aug 24, 2016

Member

Why not just this.line.trim()?

This comment has been minimized.

@princejwesley

princejwesley Aug 24, 2016

Contributor

@addaleax Updated

lib/readline.js
+ const line = this.line.replace(/^\s+|\s+$/, '');
+ // if the trimmed line is empty then return the line
+ if (line.length === 0) return this.line;
+

This comment has been minimized.

@addaleax

addaleax Aug 24, 2016

Member

Just to make sure I understand, the changes here are technically independent from the rest of the changes, right? Like, itโ€™s only in order not to clobber the history with whitespace-only lines that come from editor mode?

@addaleax

addaleax Aug 24, 2016

Member

Just to make sure I understand, the changes here are technically independent from the rest of the changes, right? Like, itโ€™s only in order not to clobber the history with whitespace-only lines that come from editor mode?

This comment has been minimized.

@princejwesley

princejwesley Aug 24, 2016

Contributor

Yes. Incorporated test to ensure that whitespace-only lines are not added in history.

@princejwesley

princejwesley Aug 24, 2016

Contributor

Yes. Incorporated test to ensure that whitespace-only lines are not added in history.

@@ -947,6 +952,10 @@ function emitKeypressEvents(stream, iface) {
if (r) {
clearTimeout(timeoutId);
+ if (iface) {
+ iface._sawKeyPress = r.length === 1;

This comment has been minimized.

@addaleax

addaleax Aug 24, 2016

Member

Is this guarding against pasted code?

@addaleax

addaleax Aug 24, 2016

Member

Is this guarding against pasted code?

This comment has been minimized.

@princejwesley

princejwesley Aug 24, 2016

Contributor

Yes

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 25, 2016

Member

There was a very small amount of red in that last run, looks completely unrelated but just to be safe: https://ci.nodejs.org/job/node-test-pull-request/3830/

Member

jasnell commented Aug 25, 2016

There was a very small amount of red in that last run, looks completely unrelated but just to be safe: https://ci.nodejs.org/job/node-test-pull-request/3830/

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 25, 2016

Member

LGTM, failures are unrelated flaky tests.

Member

jasnell commented Aug 25, 2016

LGTM, failures are unrelated flaky tests.

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Aug 26, 2016

Contributor

@addaleax I'll land this tomorrow, if there are no other questions/suggestions.

Contributor

princejwesley commented Aug 26, 2016

@addaleax I'll land this tomorrow, if there are no other questions/suggestions.

princejwesley added a commit that referenced this pull request Aug 27, 2016

repl: Auto alignment for .editor mode
When in `.editor` mode, current line whitespace prefixes
are preserved in the subsequent line. User can hit backspace
to clean the whitespace

```js
node ๐Ÿ™ˆ โ‚น node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!'); //On enter, cursor will be after 2 spaces
  _
```

PR-URL: #8241
Reviewed-By: James M Snell <jasnell@gmail.com>
@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Aug 27, 2016

Contributor

Landed in 5bef38b

Contributor

princejwesley commented Aug 27, 2016

Landed in 5bef38b

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

repl: Auto alignment for .editor mode
When in `.editor` mode, current line whitespace prefixes
are preserved in the subsequent line. User can hit backspace
to clean the whitespace

```js
node ๐Ÿ™ˆ โ‚น node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!'); //On enter, cursor will be after 2 spaces
  _
```

PR-URL: nodejs#8241
Reviewed-By: James M Snell <jasnell@gmail.com>

 Conflicts:
	lib/readline.js

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

repl: Auto alignment for .editor mode
When in `.editor` mode, current line whitespace prefixes
are preserved in the subsequent line. User can hit backspace
to clean the whitespace

```js
node ๐Ÿ™ˆ โ‚น node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!'); //On enter, cursor will be after 2 spaces
  _
```

PR-URL: #8241
Reviewed-By: James M Snell <jasnell@gmail.com>

 Conflicts:
	lib/readline.js

@Fishrock123 Fishrock123 referenced this pull request Sep 9, 2016

Merged

v6.6.0 proposal #8466

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

2016-09-14, Version 6.6.0 (Current)
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) #8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) #8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) #8174

Refs: #8428
Refs: #8457
PR-URL: #8466

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 15, 2016

2016-09-14, Version 6.6.0 (Current)
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
nodejs#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) nodejs#8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
nodejs#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
nodejs#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) nodejs#8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) nodejs#8174

Refs: nodejs#8428
Refs: nodejs#8457
PR-URL: nodejs#8466

imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 15, 2016

2016-09-14, Version 6.6.0 (Current)
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 15, 2016

2016-09-14, Version 6.6.0 (Current)
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment