repl: improve .help by silverwind · Pull Request #8519 · nodejs/node

silverwind · 2016-09-13T18:47:49Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

repl

Description of change

Added dots to .help print
Use spaces instead of tabs so there's no misalignment on terminals with a tab size other than 4
Fixed the help text for .editor

Before:

> .help
break Sometimes you get stuck, this gets you out
clear Alias for .break
editor Entering editor mode (^D to finish, ^C to cancel)
exit Exit the repl
help Show repl options
load Load JS from a file into the REPL session
save Save all evaluated commands in this REPL session to a file

After:

> .help
.break Sometimes you get stuck, this gets you out
.clear Alias for .break
.editor Enter editor mode
.exit Exit the repl
.help Show repl options
.load Load JS from a file into the REPL session
.save Save all evaluated commands in this REPL session to a file

addaleax · 2016-09-13T18:50:49Z

' '.repeat(9 - name.length)?

addaleax · 2016-09-13T18:53:24Z

I found the (^D to finish, ^C to cancel) bit helpful, especially given that @princejwesley is planning on removing “normal” multiline support?

silverwind · 2016-09-13T18:54:50Z

I find it a bit redundant there, especially because .editor prints it at the start anyways.

addaleax · 2016-09-13T18:58:38Z

… good point, I should probably get used to having to do .editor. ;)

LGTM

thefourtheye · 2016-09-13T19:05:48Z

Wait, doesn't it fail?
> ' '.repeat(-1) RangeError: Invalid count value

Yes, it will fail once we add a command with more than 10 characters. The longest right now is editor. Do you think that warrants a safeguard for future additions?

Oh, I see that should actually be 9 - name.length. Fixing that.

silverwind · 2016-09-13T19:07:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4030/

@princejwesley noticed you print

// Entering editor mode (^D to finish, ^C to cancel)

at the start of editor mode. Would you object to me changing it to

(Entering editor mode, press ^D to finish, ^C to cancel)

so it's more in line with the ^C message?

(To exit, press ^C again or type .exit)

princejwesley · 2016-09-13T19:10:39Z

@silverwind IMHO, // creates the feel that we are already in js editor.

silverwind · 2016-09-13T19:20:14Z

@princejwesley okay let's keep it
@thefourtheye fixed

new CI: https://ci.nodejs.org/job/node-test-pull-request/4031/

princejwesley · 2016-09-13T19:24:41Z

@silverwind For the .help description, why not just print this help message, print this summary or show this message ?

thefourtheye · 2016-09-13T19:42:46Z

If the length is equal to 9, then there will be no space. Why not just
constspaces=' '.repeat(Math.max(9-name.length,1));

@silverwind Find the max length programmatically. User defined commands may have bigger name 😄

Eh, I guess why not :)

- Added dots to printed commands - Use spaces instead of tabs so there's no misalignment on terminals with a tab size other than 4 - Improved the help text for .editor and .help PR-URL: nodejs#8519

silverwind · 2016-09-13T19:48:36Z

@princejwesley, @thefourtheye done and done

Also fixed a test. New CI: https://ci.nodejs.org/job/node-test-pull-request/4032/

princejwesley · 2016-09-13T19:56:40Z

@silverwind I think you missed my last comment. User defined commands may have bigger name. find the max length programmatically.

silverwind · 2016-09-13T20:07:21Z

@princejwesley Done. The amount of spaces is now calculated based on the longest command, ensuring a minimum of three spaces.

princejwesley · 2016-09-13T20:22:58Z

@@ -1262,7 +1262,7 @@ function defineDefaultCommands(repl) {
 const names = Object.keys(this.commands).sort();
 const longestNameLength = names.reduce((max, name) => {
 return Math.max(max, name.length);


@silverwind(max, name) => Math.max(max, name.length) ? is it against style guide?

It's not, but it won't fit on 80 chars so I went with the longer form.

Why not named function? Feel free to ignore it
constmaxFun=(max,name)=>Math.max(max,name.length);constlongestNameLength=names.reduce(maxFun,0);
or
constnameLengthList=names.map((n)=>n.length);constlongestNameLength=nameLengthList.reduce(Math.max,0);

TBH, I find that these reduce readability as compared to what I have now.

jasnell

LGTM

princejwesley · 2016-09-15T17:24:09Z

LGTM. I like this new github interface

imyller

LGTM

- Added dots to printed commands. - Use spaces instead of tabs so there's no misalignment on terminals with a tab size other than 4. - Improved the help text for .editor and .help. - Automatically indent command help based on the longest command. PR-URL: #8519 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

silverwind · 2016-09-16T14:53:32Z

Thanks, landed in 39fbb5a.

silverwind added the repl Issues and PRs related to the REPL subsystem. label Sep 13, 2016

addaleax reviewed Sep 13, 2016
View reviewed changes

thefourtheye reviewed Sep 13, 2016
View reviewed changes

silverwind force-pushed the repl-help-dots branch from 213394f to 93b3d8bCompare September 13, 2016 19:32

thefourtheye reviewed Sep 13, 2016
View reviewed changes

repl: improve .help message
82afbbb
- Added dots to printed commands - Use spaces instead of tabs so there's no misalignment on terminals with a tab size other than 4 - Improved the help text for .editor and .help PR-URL: nodejs#8519

silverwind force-pushed the repl-help-dots branch from 93b3d8b to 82afbbbCompare September 13, 2016 19:47

dynamically calculate amount of spaces based on command names
db73c0d

seed Array#reduce with 0
bc045e6

princejwesley reviewed Sep 13, 2016
View reviewed changes

silverwind mentioned this pull request Sep 14, 2016
Everything is on fire (major github feature update) StylishThemes/GitHub-Dark#404
Closed

jasnell approved these changes Sep 15, 2016
View reviewed changes

imyller approved these changes Sep 15, 2016
View reviewed changes

silverwind closed this Sep 16, 2016

silverwind deleted the repl-help-dots branch September 16, 2016 14:53

MylesBorins added the dont-land-on-v4.x label Oct 6, 2016

Uh oh!

Uh oh!

Conversation

silverwind commented Sep 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

silverwind commented Sep 13, 2016

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Sep 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

princejwesley commented Sep 13, 2016

Uh oh!

silverwind commented Sep 13, 2016

Uh oh!

princejwesley commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Sep 13, 2016

Uh oh!

princejwesley commented Sep 13, 2016

Uh oh!

silverwind commented Sep 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

princejwesley commented Sep 15, 2016

Uh oh!

imyller left a comment

Choose a reason for hiding this comment

Uh oh!

silverwind commented Sep 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

silverwind commented Sep 13, 2016•
edited
Loading

silverwind commented Sep 13, 2016•
edited
Loading

silverwind commented Sep 13, 2016•
edited
Loading