Skip to content
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

[v6.x backport] Repl: do not consider ... as a REPL command #14915

Closed
wants to merge 76 commits into from

Conversation

shivanth
Copy link
Contributor

@shivanth shivanth commented Aug 18, 2017

This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword.

Fixes: #14426
PR-URL: #14467
Reviewed-By: Roman Reiss me@silverwind.io
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com

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

REPL

vsemozhetbyt and others added 30 commits August 15, 2017 20:57
Also fix repl and url libs for the rule.

PR-URL: nodejs#14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

PR-URL: nodejs#14134
Fixes: nodejs#14133
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
The tests include a callback that might not be invoked but is wrapped in
common.mustCall(). Remove the common.mustCall() wrapper and add a
comment explaining that it should not be added.

Add a new test case that sets the timeout to 1ms and waits for both the
connection handler and the timeout handler to be invoked. This version
keeps the common.mustCall() wrapper intact around the connection handler
(although it's mostly semantic and not necessary for the test as the
test will certainly fail or time out if that handler isn't invoked).

PR-URL: nodejs#14380
Fixes: nodejs#11768
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#14343
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#14343
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
* Do not repeat RegExp creation in cycle.
* Use sufficient string instead of RegExp in split().

PR-URL: nodejs#13709
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
Backport-PR-URL: nodejs#14830
PR-URL: nodejs#13895
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Provide a bash script for updating ESLint in the project.

Backport-PR-URL: nodejs#14830
PR-URL: nodejs#13895
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#13946
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
If the filesystem does not support UCS2, do not run the test.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#14029
Fixes: nodejs#14028
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
In preparation for stricter indentation linting, remove the
align-multiline-assignment custom rule, as it may conflict with the
ESLint stricter indentation linting.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#14079
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#14078
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#14090
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
In anticipation of stricter indentation linting, normalize indentation
of code in parentheses.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#14125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
In preparation for stricter indentation linting and to increase code
clarity, update indentation for ternaries in lib.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#14247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
ESLint 4.x has stricter linting than previous versions. We are currently
using the legacy indentation rules in the test directory. This commit
changes the indentation of files to comply with the stricter 4.x linting
and enable stricter linting in the test directory.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#14431
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
In preparation for stricter ESLint indentation checking, fix a few
issues in sample code.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#13950
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
All linting now uses the current ESLint 4.3.0 indentation linting.
Remove legacy indentation rules.

Backport-PR-URL: nodejs#14835
PR-URL: nodejs#14515
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
As indicated by the FIXME comment, this macro guard is no longer needed.

PR-URL: nodejs#12638
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
* Make common.skip() exit.

  Also add common.printSkipMessage() for partial skips.

* Don't make needless things before skip

Backport-PR-URL: nodejs#14838
PR-URL: nodejs#14021
Fixes: nodejs#14016
Reviewed-By: Refael Ackermann <refack@gmail.com>
Backport-PR-URL: nodejs#14842
PR-URL: nodejs#13900
Fixes: nodejs#13882
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
* rename :exit to :distexit

Backport-PR-URL: nodejs#14842
PR-URL: nodejs#13969
Refs: nodejs#13900 (review)
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Handle spaces in the path to python.exe, in case it is installed
under some directory like "C:\Program Files".

Backport-PR-URL: nodejs#14842
PR-URL: nodejs#14546
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Instead of generating string concatenation, generate a template literal.
This is mostly useful as a pre-emptive measure for avoiding problems
when (if?) we enable the prefer-template lint rule in the test
directory.

PR-URL: nodejs#14094
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Explain the behavior of `fs.open()` under win32 that file path contains
some characters and add some test cases for them.

< (less than)
> (greater than)
: (colon)
" (double quote)
/ (forward slash)
\ (backslash)
| (vertical bar or pipe)
? (question mark)
* (asterisk)

PR-URL: nodejs#13875
Refs: nodejs#13868
Refs: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
Refs: https://msdn.microsoft.com/en-us/library/windows/desktop/bb540537.aspx
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
If the first parameter of `request.end` `data` is specified, it should
be equivalent to calling `request.write(data, encoding)` (not
`response.write(data, encoding)`) followed by `request.end(callback)`.

This mistake was introduced in commit
14b3aab:

    date: 28 November 2015 at 7:30:32 AM GMT+8
    author: jpersson <jonathan.persson@creuna.se>
    committer: James M Snell <jasnell@gmail.com>
    summary: doc: add links and backticks around names

PR-URL: nodejs#14126
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: nodejs#13576
Fixes: nodejs#13197
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
If allowHalfOpen is set to false, the stream will automatically end the
writable side when the readable side ends, but not the other way around.

PR-URL: nodejs#14127
Fixes: nodejs#4044
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
3c92ca2 should have had tests
to go along with it. This adds tests for the following functions:

* `process.geteuid()`
* `process.seteuid()`
* `process.getegid()`
* `process.setegid()`

PR-URL: nodejs#14091
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This change removes `common.noop` from the Node.js internal testing
common module.

Over the last few weeks, I've grown to dislike the `common.noop`
abstraction.

First, new (and experienced) contributors are unaware of it and so it
results in a large number of low-value nits on PRs. It also increases
the number of things newcomers and infrequent contributors have to be
aware of to be effective on the project.

Second, it is confusing. Is it a singleton/property or a getter? Which
should be expected? This can lead to subtle and hard-to-find bugs. (To
my knowledge, none have landed on master. But I also think it's only a
matter of time.)

Third, the abstraction is low-value in my opinion. What does it really
get us? A case could me made that it is without value at all.

Lastly, and this is minor, but the abstraction is wordier than not using
the abstraction. `common.noop` doesn't save anything over `() => {}`.

So, I propose removing it.

PR-URL: nodejs#12822
Backport-PR-URL: nodejs#14174
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
alexbostock and others added 8 commits August 16, 2017 11:42
PR-URL: nodejs#14546
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
This commit calls require on a shared library that is not declared
as a node module, and therefore does not register properly.

PR-URL: nodejs#13954
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
* block scope test cases
* clean up global leaks in individual test cases
* enable global variable leak checking
* remove console.error() statements

PR-URL: nodejs#14536
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
* use common.mustCall() instead of exit handler
* use execSync instead of exec so test is reliable under load
* move from sequential to parallel

PR-URL: nodejs#14541
Fixes: nodejs#11826
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
This is a very very minor change.

PR-URL: nodejs#14587
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: nodejs#14692
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#14417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Refael Ackermann <refack@gmail.com>
This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword.

Fixes: nodejs#14426
PR-URL: nodejs#14467
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@nodejs-github-bot nodejs-github-bot added repl Issues and PRs related to the REPL subsystem. v6.x labels Aug 18, 2017
lib/repl.js Outdated
if (self.parseREPLKeyword(keyword, rest) === true) {
return;
}
if (!self.bufferedCommand) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is pushing it in under the first if is equivalent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a return anyways within the first if. I found it this way in the master branch. I had the same question too :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do we have to keep it as it is... ?

Copy link
Contributor

@refack refack Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT for (cmd == false & self.bufferedCommand == true) it will act differently.
I think since for v6.x stability if very important, so we should keep the original if (as long as it still passes the test).
I started a CI job to see if any test will fail. If you can think if a test case that creates (cmd == false & self.bufferedCommand == true) maybe we should add it.

@refack
Copy link
Contributor

refack commented Aug 18, 2017

@shivanth thank for doing the backport 🥇
Since you're a veteran contributor now I've commented in "short-form" don't read too much into it.

@refack
Copy link
Contributor

refack commented Aug 19, 2017

@shivanth
Copy link
Contributor Author

The build failed ?

@refack
Copy link
Contributor

refack commented Aug 20, 2017

The build failed ?

No build is Ok, just some infrastructure problems.
I've resumed it: https://ci.nodejs.org/job/node-test-commit/11920/

@shivanth
Copy link
Contributor Author

Let's revert it back then ? to a single if ? Can't think of a test case though..

@refack
Copy link
Contributor

refack commented Aug 21, 2017

Let's revert it back then ? to a single if ? Can't think of a test case though..

I think so, just to be safe.

Meanwhile I'll try to think of a test-case.

@refack
Copy link
Contributor

refack commented Aug 22, 2017

I have an almost working test:

{
    input: '1\n...',
    output: `\n...\n^^^\nSyntaxError: Unexpected token ...\n\n${terminalCode}`,
    event: { ctrl: true, name: 'd' }
}

Using run in

function run({input, output, event, checkTerminalCodes = true}) {

@shivanth this is the diff https://www.diffchecker.com/xm2DPnfN (expected on the left, actual on the right), if you have the time to try this out.

@shivanth
Copy link
Contributor Author

Yup, let me take a look at it..

@MylesBorins MylesBorins force-pushed the v6.x-staging branch 2 times, most recently from aaf4e13 to 31f572c Compare September 5, 2017 16:50
@MylesBorins
Copy link
Contributor

It appears to me that this is a re-implementation rather than just a backport. Can we get some more reviews on it?

also needs a rebase

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Largely rubber stamp LGTM

MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword.

Fixes: #14426
Backport-PR-URL: #14915
PR-URL: #14467
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins
Copy link
Contributor

landed in 362a7c0

please lmk if this didn't land properly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.