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.

Sign up for GitHub

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

Jump to bottom

readline: refactor readline module #12755

Closed
wants to merge 5 commits into from
Closed

readline: refactor readline module #12755

wants to merge 5 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 30, 2017

Refactor parts of the readline module

  • Move to more efficient module.exports = {} pattern
  • Restructure imports and other minor code cleanups
  • Move CSI (escape codes) into internal/readline for easier maintenance
  • Improve test coverage
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

readline

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Apr 30, 2017
@jasnell jasnell changed the title Refactor readline readline: refactor readline module Apr 30, 2017
@jasnell
Copy link
Member Author

jasnell commented Apr 30, 2017

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

@refack
Copy link
Contributor

refack commented Apr 30, 2017

IMHO in order not to conflict with @12711 change the variable function to arrows

@refack
Copy link
Contributor

refack commented Apr 30, 2017

P.S. freeBSD seems unrelated:

731	parallel/test-net-connect-local-error	
duration_ms	0.364
severity	fail
stack	
assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: undefined !== 12346 in Error: connect EADDRINUSE 127.0.0.1:12347 - Local (127.0.0.1:12346)
    at Socket.onError (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-net-connect-local-error.js:13:10)
    at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common.js:461:15)
    at emitOne (events.js:115:13)
    at Socket.emit (events.js:210:7)
    at emitErrorNT (net.js:1302:8)
    at _combinedTickCallback (internal/process/next_tick.js:81:11)
    at process._tickCallback (internal/process/next_tick.js:105:9)

addaleax
addaleax requested changes Apr 30, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, got one tiny correction though ;)

lib/internal/readline.js Outdated
@@ -7,23 +7,38 @@
const ansi =
/[\u001b\u009b][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-ORZcf-nqry=><]/g;

const kCSI = '\x1b';
Copy link
Member

Choose a reason for hiding this comment

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

The CSI would be \x1b[ (2 chars) or \x9b, this is just an escape character, so it would probably be a good idea to rename this to kEscape, or use \x1b[ here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point :-)

jasnell added 2 commits May 1, 2017 10:49
@jasnell
readline: move escape codes into internal/readline
291a464 
Moves escape codes into internal/readline for easier management.
@jasnell
test: expand test coverage of readline
a5521a6
@jasnell jasnell force-pushed the refactor-readline branch from 58cdaec to a5521a6 Compare May 1, 2017 18:04
@jasnell
Copy link
Member Author

jasnell commented May 1, 2017

@addaleax ... updated!

@addaleax
Copy link
Member

addaleax commented May 3, 2017

Fresh CI: https://ci.nodejs.org/job/node-test-commit/9607/

@addaleax
Copy link
Member

addaleax commented May 7, 2017

Landed in a398516...d37f27a

@addaleax addaleax closed this May 7, 2017
addaleax pushed a commit that referenced this pull request May 7, 2017
@jasnell @addaleax
readline: use module.exports = {}
9318f82 
PR-URL: #12755
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request May 7, 2017
@jasnell @addaleax
readline: use module.exports = {} on internal/readline
392a898 
PR-URL: #12755
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request May 7, 2017
@jasnell @addaleax
readline: multiple code cleanups
4ac7a68 
Variety of code maintenance updates, cleanups

PR-URL: #12755
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request May 7, 2017
@jasnell @addaleax
readline: move escape codes into internal/readline
4c070d4 
Moves escape codes into internal/readline for easier management.

PR-URL: #12755
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request May 7, 2017
@jasnell @addaleax
test: expand test coverage of readline
d37f27a 
PR-URL: #12755
Reviewed-By: Anna Henningsen <[email protected]>
@YurySolovyov YurySolovyov mentioned this pull request May 12, 2017
Closed
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@jasnell
readline: use module.exports = {}
5f25bdf 
PR-URL: nodejs#12755
Reviewed-By: Anna Henningsen <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@jasnell
readline: use module.exports = {} on internal/readline
5ec24d8 
PR-URL: nodejs#12755
Reviewed-By: Anna Henningsen <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@jasnell
readline: multiple code cleanups
1a6c82c 
Variety of code maintenance updates, cleanups

PR-URL: nodejs#12755
Reviewed-By: Anna Henningsen <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@jasnell
readline: move escape codes into internal/readline
6d50843 
Moves escape codes into internal/readline for easier management.

PR-URL: nodejs#12755
Reviewed-By: Anna Henningsen <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@jasnell
test: expand test coverage of readline
7a034c5 
PR-URL: nodejs#12755
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Very possibly not worth backporting, your call @jasnell .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasnell @refack @addaleax @gibfahn @TimothyGu @nodejs-github-bot