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

repl: refactor lib/repl.js #9374

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 31, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change
  • remove unnecessary backslash (\) escaping in regular expressions
  • favor === over ==
  • multiline arrays indentation consistent with other indentation

@Trott Trott added the repl Issues and PRs related to the REPL subsystem. label Oct 31, 2016
Copy link
Contributor

@princejwesley princejwesley left a comment

Choose a reason for hiding this comment

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

convertToContext is unused function. Do we need to correct it?

@rvagg
Copy link
Member

rvagg commented Oct 31, 2016

lgtm, but does pulling that array in allow you to pull entries up to previous line and satisfy line length restrictions?

@@ -783,7 +783,7 @@ ArrayStream.prototype.writable = true;
ArrayStream.prototype.resume = function() {};
ArrayStream.prototype.write = function() {};

const requireRE = /\brequire\s*\(['"](([\w\.\/-]+\/)?([\w\.\/-]*))/;
const requireRE = /\brequire\s*\(['"](([\w./-]+\/)?([\w./-]*))/;
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like a behavior change?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Fishrock123 No. we don't need to escape characters inside character class [``] except for - in non-tail position.

@Trott
Copy link
Member Author

Trott commented Oct 31, 2016

@rvagg asked:

lgtm, but does pulling that array in allow you to pull entries up to previous line and satisfy line length restrictions?

It was possible to do that even without reducing the indentation, so yes, definitely possible. I didn't want to do it because I wanted to avoid hard-to-read diff that might be criticized as churn. A whitespace-only change is annoying enough, but at least it's easy to comprehend the diff. :-)

However, if you or anyone else feels strongly that it's important to re-wrap the lines, I can do that. (Honestly, I'd prefer one-item-to-a-line over put-as-many-as-you-can-on-a-line because at least adding and deleting items becomes very clear in diffs. But that's just my preference.)

@Trott
Copy link
Member Author

Trott commented Oct 31, 2016

@princejwesley wrote:

convertToContext is unused function. Do we need to correct it?

I guess we don't need to, but I figured I might as well remove all the no-op escapes while I was at it.

@rvagg
Copy link
Member

rvagg commented Oct 31, 2016

your call on the array @Trott, no strong feelings from me either way

@@ -1341,8 +1341,8 @@ function regexpEscape(s) {
// TODO(princejwesley): Remove it prior to v8.0.0 release
// Reference: https://github.com/nodejs/node/pull/7829
REPLServer.prototype.convertToContext = util.deprecate(function(cmd) {
const scopeVar = /^\s*var\s*([_\w\$]+)(.*)$/m;
const scopeFunc = /^\s*function\s*([_\w\$]+)/;
const scopeVar = /^\s*var\s*([_\w$]+)(.*)$/m;

Choose a reason for hiding this comment

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

Remove _ while you’re at it? It’s included in \w.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Thanks!

* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation
@Trott
Copy link
Member Author

Trott commented Nov 2, 2016

@Trott
Copy link
Member Author

Trott commented Nov 3, 2016

CI failures are infra/build related. Landing momentarily....

Trott added a commit to Trott/io.js that referenced this pull request Nov 3, 2016
* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation

PR-URL: nodejs#9374
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott
Copy link
Member Author

Trott commented Nov 3, 2016

Landed in 5cbb7a8

@Trott Trott closed this Nov 3, 2016
evanlucas pushed a commit that referenced this pull request Nov 7, 2016
* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation

PR-URL: #9374
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins
Copy link
Contributor

@Trott do you want to backport this? it will need to be done manually

Trott added a commit to Trott/io.js that referenced this pull request Nov 22, 2016
* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation

PR-URL: nodejs#9374
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Trott added a commit to Trott/io.js that referenced this pull request Nov 22, 2016
* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation

PR-URL: nodejs#9374
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott
Copy link
Member Author

Trott commented Nov 22, 2016

@thealphanerd Backports done in #9747 and #9748

MylesBorins pushed a commit that referenced this pull request Dec 8, 2016
* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation

PR-URL: #9374
Ref: #9747
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation

PR-URL: #9374
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation

PR-URL: #9374
Ref: #9747
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
* remove unnecessary backslash (`\`) escaping in regular expressions
* favor `===` over `==`
* multiline arrays indentation consistent with other indentation

PR-URL: #9374
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
This was referenced Dec 21, 2016
@Trott Trott deleted the repl-refactor branch January 13, 2022 22:44
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.

9 participants