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: make tab size configurable #31318

Closed
wants to merge 1 commit into from
Closed

readline: make tab size configurable #31318

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

This adds the tabSize option to readline to allow different tab
sizes. It also cleans up some code and unifies the linebreak RegExp.

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

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jan 11, 2020
@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 11, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
bnoordhuis
bnoordhuis reviewed Jan 12, 2020
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

The historySize changes should be a separate commit and arguably a separate PR because it's unrelated.

lib/readline.js Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR BridgeAR force-pushed the 2020-01-11-readline-make-tab-size-configurable branch from 60f32a5 to 77effd2 Compare January 12, 2020 20:14
@BridgeAR
Copy link
Member Author

@bnoordhuis I removed the refactoring.

@BridgeAR BridgeAR force-pushed the 2020-01-11-readline-make-tab-size-configurable branch from 77effd2 to 5f1947d Compare January 12, 2020 21:04
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 12, 2020 •
edited by BridgeAR
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/28419/ ✔️

@BridgeAR
Copy link
Member Author

@nodejs/readline @nodejs/repl PTAL (this is the basis to add the same option to the repl).

@Trott
Copy link
Member

Trott commented Jan 14, 2020

Not opposed to this, but curious what the motivation is. Is this something you've needed but didn't have? Is it something someone has asked for?

@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 14, 2020 •
edited
Loading

@Trott neither. The reason is that it's possible to change the tab size in terminals.

@BridgeAR
Copy link
Member Author

@nodejs/repl @nodejs/readline this could use some reviews.

mcollina
mcollina reviewed Jan 20, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I would prefer if this did not edit existings tests as well, it makes the review harder.

@BridgeAR
Copy link
Member Author

I updated the tests to be independent but I want to note that we just copy 95% of the test code all the time.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

antsmartian
antsmartian approved these changes Jan 21, 2020
View reviewed changes
lib/readline.js Show resolved Hide resolved
@BridgeAR BridgeAR force-pushed the 2020-01-11-readline-make-tab-size-configurable branch from d7eddc1 to 0b9ec1d Compare January 22, 2020 14:58
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 5, 2020
addaleax
addaleax approved these changes Feb 5, 2020
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.

This needs a rebase

@BridgeAR BridgeAR force-pushed the 2020-01-11-readline-make-tab-size-configurable branch from 0b9ec1d to 27983d4 Compare February 6, 2020 19:46
@BridgeAR
readline: make tab size configurable
c7571e2 
This adds the `tabSize` option to readline to allow different tab
sizes.
@BridgeAR BridgeAR force-pushed the 2020-01-11-readline-make-tab-size-configurable branch from 27983d4 to c7571e2 Compare February 6, 2020 19:48
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

addaleax pushed a commit that referenced this pull request Feb 7, 2020
@BridgeAR @addaleax
readline: make tab size configurable
f9a27ea 
This adds the `tabSize` option to readline to allow different tab
sizes.

PR-URL: #31318
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

addaleax commented Feb 7, 2020

Landed in f9a27ea

@addaleax addaleax closed this Feb 7, 2020
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@BridgeAR @codebytere
readline: make tab size configurable
148dfde 
This adds the `tabSize` option to readline to allow different tab
sizes.

PR-URL: #31318
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
codebytere added a commit that referenced this pull request Feb 18, 2020
@codebytere
2020-02-18, Version 13.9.0 (Current)
1f0f58c 
Notable changes:

* async_hooks
  * add executionAsyncResource (Matteo Collina) #30959
* crypto
  * add crypto.diffieHellman (Tobias Nießen) #31178
  * add DH support to generateKeyPair (Tobias Nießen) #31178
  * simplify DH groups (Tobias Nießen) #31178
  * add key type 'dh' (Tobias Nießen) #31178
* test
  * skip keygen tests on arm systems (Tobias Nießen) #31178
* perf_hooks
  * add property flags to GCPerformanceEntry (Kirill Fomichev) #29547
* process
  * report ArrayBuffer memory in `memoryUsage()` (Anna Henningsen) #31550
* readline
  * make tab size configurable (Ruben Bridgewater) #31318
* report
  * add support for Workers (Anna Henningsen) #31386
* worker
  * add ability to take heap snapshot from parent thread (Anna Henningsen) #31569
* added new collaborators
  * add ronag to collaborators (Robert Nagy) #31498

PR-URL: #31837
codebytere added a commit that referenced this pull request Feb 18, 2020
@codebytere
2020-02-18, Version 13.9.0 (Current)
0c3c0e7 
Notable changes:

* async_hooks
  * add executionAsyncResource (Matteo Collina) #30959
* crypto
  * add crypto.diffieHellman (Tobias Nießen) #31178
  * add DH support to generateKeyPair (Tobias Nießen) #31178
  * simplify DH groups (Tobias Nießen) #31178
  * add key type 'dh' (Tobias Nießen) #31178
* test
  * skip keygen tests on arm systems (Tobias Nießen) #31178
* perf_hooks
  * add property flags to GCPerformanceEntry (Kirill Fomichev) #29547
* process
  * report ArrayBuffer memory in `memoryUsage()` (Anna Henningsen) #31550
* readline
  * make tab size configurable (Ruben Bridgewater) #31318
* report
  * add support for Workers (Anna Henningsen) #31386
* worker
  * add ability to take heap snapshot from parent thread (Anna Henningsen) #31569
* added new collaborators
  * add ronag to collaborators (Robert Nagy) #31498

PR-URL: #31837
@targos targos mentioned this pull request Apr 18, 2020
Merged
2 tasks
@targos
Copy link
Member

targos commented Apr 25, 2020

Depends on #31423 to land on v12.x

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

@lundibundi lundibundi lundibundi approved these changes

@bnoordhuis bnoordhuis bnoordhuis left review comments

@mcollina mcollina mcollina left review comments

@antsmartian antsmartian antsmartian approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
readline Issues and PRs related to the built-in readline module. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@BridgeAR @nodejs-github-bot @Trott @addaleax @targos @mcollina @bnoordhuis @jasnell @antsmartian @lundibundi