doc: some fixes in repl.md #10160

vsemozhetbyt · 2016-12-06T23:46:21Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, repl

Description of change

var => let / const.
White space unification (add an infix space in an argument list; change > into > (with a trailing space) in code bits and output examples; explicitly clarify that default REPL prompt contains a trailing space).
Fix an output example (make _ reassignment example match more with the current output; extend the example for more clarity).
Fix a function name (eval => myEval to not shadow the global eval).
Name anonymous functions.
Add the valid link for curl(1) (The current autoinserted link leads to 404 page).

Add an infix space in an argument list. Change `>` into `> ` in code bits and output examples. Explicitly clarify that default REPL prompt contains a trailing space.

Make `_` reassignment example match more with the current output. Extend the example for more clarity.

`eval` => `myEval` to not shadow the global `eval`

`options` in the `repl.start([options])` can be a string.

The current autoinserted link leads to 404 page.

jasnell · 2016-12-07T00:11:52Z

/cc @nodejs/documentation

eljefedelrodeodeljefe · 2016-12-07T00:23:50Z

doc/api/repl.md

 replServer.defineCommand('sayhello', {
  help: 'Say hello',
-  action: function(name) {
+  action: function sayHello(name) {


doesn't add any value. I'd rather just leave at is it, but do action: function (name) { for coding style's sake

I've just thought the docs should promote the style appropriate for the code base:

https://github.com/nodejs/node/pulls?q=is%3Apr+name+anonymous+functions+is%3Aclosed

But if I'm wrong, I'll roll back these two with just space insertion.

I understand. Also I do not have strong feelings about this, but in the /lib case it would hae value since stack traces might look nicer. Here showing APIs is the prime thing to do. E.g. you will never see full error handling in programming books. Again. I don't mind that's why I left it as a comment.

I see. Well, if nobody will support the additions, I will change as you advise.

Yeah, but I think we only try to do that in cases where the name is not inferred (well) by V8. Here, .name would just be action without the hint, I think that’s okay.

If you want to change style here, I’d be +1 on just the action(name) { shorthand notation

Consider also a method shorthand.

O, I have not seen @addaleax comment)

eljefedelrodeodeljefe · 2016-12-07T00:24:54Z

doc/api/repl.md

    this.lineParser.reset();
    this.bufferedCommand = '';
    console.log(`Hello, ${name}!`);
    this.displayPrompt();
  }
 });
-replServer.defineCommand('saybye', function() {
+replServer.defineCommand('saybye', function sayBye() {


same as above

Consider also an arrow function.

eljefedelrodeodeljefe · 2016-12-07T00:25:42Z

doc/api/repl.md

-  * `prompt` {String} The input prompt to display. Defaults to `> `.
+* `options` {Object | String}
+  * `prompt` {String} The input prompt to display. Defaults to `> `
+    (with a trailing space).


(...)-content doesn't add any value.

The trailing space is not shown in the rendered page, it is wiped out. Is this not somehow confusing?

eljefedelrodeodeljefe · 2016-12-07T00:27:21Z

doc/api/repl.md

@@ -411,6 +417,8 @@ added: v0.1.91
    `SIGINT` is received, i.e. `Ctrl+C` is pressed. This cannot be used together
    with a custom `eval` function. Defaults to `false`.


maybe add reference to your new name here

This eval is used in multiple places as an abstraction. It is just in the code that it shadows global.

eljefedelrodeodeljefe

LGTM, with comments, minus one content issue I would rather want to check or in a separate PR.

eljefedelrodeodeljefe · 2016-12-07T00:29:19Z

doc/api/repl.md

@@ -411,6 +417,8 @@ added: v0.1.91
    `SIGINT` is received, i.e. `Ctrl+C` is pressed. This cannot be used together
    with a custom `eval` function. Defaults to `false`.

+If `options` is a string, then it specifies the input prompt.
+


Don't know whether this is true. If so it would be a major change in documentation, since also the type is just displaying <Object>

Well, it is almost documented in some examples. See:

https://github.com/nodejs/node/blame/master/doc/api/repl.md#L101
https://github.com/nodejs/node/blame/master/doc/api/repl.md#L120

If it is not official API, maybe we should change these fragments too?

This is actually how it works, although I don't know how "official" it is. See repl.start and the REPLServer constructor. I do see examples of this usage all over the place. In addition to what @vsemozhetbyt referenced there is another example here.

eljefedelrodeodeljefe · 2016-12-07T00:33:02Z

Thanks for the PR in any case, since there are apparently still a lot things to do in this file.

Replaced with an object shorthand and an arrow function.

vsemozhetbyt · 2016-12-07T01:42:14Z

I've replaced the anonymous functions in more concise ways, maybe they are better now. As to options addition, if nobody else supports, I will revert this change for a separate PR.

test-buffer-creation-regression is flaky on some SmartOS hosts in CI, timing out. Move to sequential so it does not compete with other tests for resources. Reduce three test cases to just the one needed to identify the regression. PR-URL: #10161 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

- changes var to const/let - changes assert.equal to assert.strictEqual PR-URL: #9947 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently when running make node_g the following error is displayed: if [ ! -r node -o ! -L ]; then ln -fs out/Debug/node node_g; fi /bin/sh: line 0: [: argument expected It looks like there was a typo for the NODE_EXE where node became lowercase instead of uppercase. Ref: #9827 PR-URL: #10153 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

lance · 2016-12-07T16:19:27Z

doc/api/repl.md

-`global` scope.
+declared either implicitly or using the `const` or `let` keywords
+are declared at the `global` scope (as well as with the `var` keyword
+outside of functions).



This seems like a cumbersome distinction between const and var, and `let. Couldn't it just be something like this?

"variables declared either implicitly, or using the const, let, or var keywords are declared at the global scope."

Especially since the phrase is immediately preceded by "Unless otherwise scoped within blocks or functions".

@lance But it is exactly this remark — "Unless otherwise scoped within blocks or functions" — that prevents placing const, var, and let in the same list: var is not block scoped. Maybe you or somebody else could propose the whole clear concise sentence there? I am not a native speaker, so I am a bit clumsy there.

var is not block scoped

The const and let declarations are block scoped, and the var declaration is function scoped. I think this sentence covers both. I'm not sure if I have a wording that is clearer.

Unless otherwise scoped within blocks or functions, variables declared either implicitly, or using the const, let, or var keywords are declared at the global scope.

Thank you. Done.

Silence coverity warnings about the return value not being checked. PR-URL: #10126 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@chromium.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #9899 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

- Make URLSearchParams constructor spec-compliant - Strip leading `?` in URL#search's setter - Spec-compliant iterable interface - More precise handling of update steps as mandated by the spec - Add class strings to URLSearchParams objects and their prototype - Make sure `this instanceof URLSearchParams` in methods Also included are relevant tests from W3C's Web Platform Tests (https://github.com/w3c/web-platform-tests/tree/master/url). Fixes: #9302 PR-URL: #9484 Reviewed-By: James M Snell <jasnell@gmail.com>

setTimeout at 49:5 requires two arguments. On lines 72 and 73 changed assert.equal() to assert.strictEqual(). PR-URL: #10003 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

In this change, the setTimeout needed a second argument, so I set that value to 1. In addition, I changed the assertion to be a strictEquals instead of equals. I changed the var declarations to const in this test. PR-URL: #9889 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #10168 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

lance · 2016-12-07T19:26:06Z

LGTM

- use const and let for variables - replace assert.equal with assert.strictEqual PR-URL: #10167 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

* use common.mustCall() to confirm number of uncaught exceptions * var -> const * specify duration of 1ms for setTimeout() and setInterval() PR-URL: #10188 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

`options` in the `repl.start([options])` can be a string. Ref: #10160 PR-URL: #10221 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

In test-http-incoming-pipelined-socket-destory: * setTimeout() with no duration -> setImmediate() * eliminate unneeded exit listener * use common.mustCall() * var -> const/let PR-URL: #10189 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

PR-URL: #10190 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

vsemozhetbyt · 2016-12-12T21:59:29Z

Oh, I am sorry. It seems I've caused a merge conflict by #10221. What should I do now?

* use common.mustCall() where appropriate * Buffer.allocUnsafe() -> Buffer.alloc() * do crypto check before loading any additional modules * specify 1ms duration for `setTimeout()` PR-URL: #10225 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

lance · 2016-12-12T22:20:39Z

@vsemozhetbyt you should rebase this PR and push those changes. This happened because I landed #10221.

Add an infix space in an argument list. Change `>` into `> ` in code bits and output examples. Explicitly clarify that default REPL prompt contains a trailing space.

Make `_` reassignment example match more with the current output. Extend the example for more clarity.

`eval` => `myEval` to not shadow the global `eval`

`options` in the `repl.start([options])` can be a string.

The current autoinserted link leads to 404 page.

Replaced with an object shorthand and an arrow function.

It will be postponed for more careful evaluating in a separate PR.

vsemozhetbyt · 2016-12-12T22:47:57Z

Sorry, it seems I've messed this up. I will close now and will redo from scratch(

`options` in the `repl.start([options])` can be a string. Ref: #10160 PR-URL: #10221 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

`options` in the `repl.start([options])` can be a string. Ref: nodejs#10160 PR-URL: nodejs#10221 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

`options` in the `repl.start([options])` can be a string. Ref: #10160 PR-URL: #10221 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

vsemozhetbyt added 7 commits December 7, 2016 00:49

doc: var => let / const in repl.md

9615910

doc: white space unification in repl.md

3e18789

Add an infix space in an argument list. Change `>` into `> ` in code bits and output examples. Explicitly clarify that default REPL prompt contains a trailing space.

doc: fix an output example in repl.md

743f84d

Make `_` reassignment example match more with the current output. Extend the example for more clarity.

doc: fix a function name in repl.md

a7501f2

`eval` => `myEval` to not shadow the global `eval`

doc: name anonymous functions in repl.md

402c625

doc: document argument variant in repl.md

e20c785

`options` in the `repl.start([options])` can be a string.

doc: add the valid link for curl(1) in repl.md

4357059

The current autoinserted link leads to 404 page.

nodejs-github-bot added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. labels Dec 6, 2016

eljefedelrodeodeljefe reviewed Dec 7, 2016

View reviewed changes

eljefedelrodeodeljefe suggested changes Dec 7, 2016

View reviewed changes

doc: replace anonymous functions in repl.md

9c7054b

Replaced with an object shorthand and an arrow function.

Trott and others added 3 commits December 7, 2016 07:35

test: refactor test-dgram-bind-default-address

da077ee

- changes var to const/let - changes assert.equal to assert.strictEqual PR-URL: #9947 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

lance reviewed Dec 7, 2016

View reviewed changes

bnoordhuis and others added 7 commits December 7, 2016 17:51

test: use const and strictEqual in test-os-homedir-no-envvar

1b25214

PR-URL: #9899 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

doc: removing extra space in README

586b787

PR-URL: #10168 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

doc: reword variable declaration note in repl.md

df4018e

edsadr and others added 5 commits December 12, 2016 11:31

test: add ES6 and strictEqual to test-fs-truncate

a8e8708

- use const and let for variables - replace assert.equal with assert.strictEqual PR-URL: #10167 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

doc: document argument variant in the repl.md

50cb3a3

`options` in the `repl.start([options])` can be a string. Ref: #10160 PR-URL: #10221 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

test: refactor assert.equal, update syntax to ES6

10891a1

PR-URL: #10190 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

vsemozhetbyt added 11 commits December 13, 2016 00:28

doc: var => let / const in repl.md

5f18b40

doc: white space unification in repl.md

bed9aae

Add an infix space in an argument list. Change `>` into `> ` in code bits and output examples. Explicitly clarify that default REPL prompt contains a trailing space.

doc: fix an output example in repl.md

9bef034

Make `_` reassignment example match more with the current output. Extend the example for more clarity.

doc: fix a function name in repl.md

67c2a7d

`eval` => `myEval` to not shadow the global `eval`

doc: name anonymous functions in repl.md

d665e39

doc: document argument variant in repl.md

5f31da7

`options` in the `repl.start([options])` can be a string.

doc: add the valid link for curl(1) in repl.md

d5861ad

The current autoinserted link leads to 404 page.

doc: replace anonymous functions in repl.md

0c32eb5

Replaced with an object shorthand and an arrow function.

doc: reword variable declaration note in repl.md

936263e

doc: revert documenting argument form in repl.md

588a5d5

It will be postponed for more careful evaluating in a separate PR.

doc: resolved merge conflict

10c0137

nodejs-github-bot added the dont-land-on-v7.x label Dec 12, 2016

vsemozhetbyt closed this Dec 12, 2016

vsemozhetbyt deleted the repl.md branch December 12, 2016 22:49

vsemozhetbyt mentioned this pull request Dec 13, 2016

doc: some fixes in repl.md #10244

Closed

2 tasks

mscdex removed the dont-land-on-v7.x label Jan 13, 2017

targos pushed a commit that referenced this pull request Jan 28, 2017

doc: document argument variant in the repl.md

5880804

`options` in the `repl.start([options])` can be a string. Ref: #10160 PR-URL: #10221 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 8, 2017

doc: document argument variant in the repl.md

daca360

`options` in the `repl.start([options])` can be a string. Ref: #10160 PR-URL: #10221 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017

doc: document argument variant in the repl.md

8c487de

`options` in the `repl.start([options])` can be a string. Ref: #10160 PR-URL: #10221 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: some fixes in repl.md #10160

doc: some fixes in repl.md #10160

vsemozhetbyt commented Dec 6, 2016 •

edited

Loading

jasnell commented Dec 7, 2016

eljefedelrodeodeljefe Dec 7, 2016

vsemozhetbyt Dec 7, 2016

eljefedelrodeodeljefe Dec 7, 2016

vsemozhetbyt Dec 7, 2016

addaleax Dec 7, 2016

vsemozhetbyt Dec 7, 2016 •

edited

Loading

vsemozhetbyt Dec 7, 2016

eljefedelrodeodeljefe Dec 7, 2016

vsemozhetbyt Dec 7, 2016

eljefedelrodeodeljefe Dec 7, 2016

vsemozhetbyt Dec 7, 2016

eljefedelrodeodeljefe Dec 7, 2016

vsemozhetbyt Dec 7, 2016 •

edited

Loading

eljefedelrodeodeljefe left a comment •

edited

Loading

eljefedelrodeodeljefe Dec 7, 2016 •

edited

Loading

vsemozhetbyt Dec 7, 2016 •

edited

Loading

lance Dec 7, 2016

eljefedelrodeodeljefe commented Dec 7, 2016

vsemozhetbyt commented Dec 7, 2016 •

edited

Loading

lance Dec 7, 2016

vsemozhetbyt Dec 7, 2016 •

edited

Loading

lance Dec 7, 2016

vsemozhetbyt Dec 7, 2016

lance commented Dec 7, 2016

vsemozhetbyt commented Dec 12, 2016 •

edited

Loading

lance commented Dec 12, 2016

vsemozhetbyt commented Dec 12, 2016

		@@ -411,6 +417,8 @@ added: v0.1.91
		`SIGINT` is received, i.e. `Ctrl+C` is pressed. This cannot be used together
		with a custom `eval` function. Defaults to `false`.

doc: some fixes in repl.md #10160

doc: some fixes in repl.md #10160

Conversation

vsemozhetbyt commented Dec 6, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

eljefedelrodeodeljefe left a comment • edited Loading

Choose a reason for hiding this comment

eljefedelrodeodeljefe Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eljefedelrodeodeljefe commented Dec 7, 2016

vsemozhetbyt commented Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance commented Dec 7, 2016

vsemozhetbyt commented Dec 12, 2016 • edited Loading

lance commented Dec 12, 2016

vsemozhetbyt commented Dec 12, 2016

vsemozhetbyt commented Dec 6, 2016 •

edited

Loading

vsemozhetbyt Dec 7, 2016 •

edited

Loading

vsemozhetbyt Dec 7, 2016 •

edited

Loading

eljefedelrodeodeljefe left a comment •

edited

Loading

eljefedelrodeodeljefe Dec 7, 2016 •

edited

Loading

vsemozhetbyt Dec 7, 2016 •

edited

Loading

vsemozhetbyt commented Dec 7, 2016 •

edited

Loading

vsemozhetbyt Dec 7, 2016 •

edited

Loading

vsemozhetbyt commented Dec 12, 2016 •

edited

Loading