GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — readline: make tab size configurable by BridgeAR · Pull Request #31318 · nodejs/node

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Jan 11, 2020

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/readline.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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

BridgeAR commented Jan 12, 2020

Copy link
Copy Markdown
Member Author

@bnoordhuis I removed the refactoring.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Jan 13, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott reviewed Jan 14, 2020
View reviewed changes
Comment thread test/parallel/test-readline-interface.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Jan 14, 2020

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Jan 14, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Jan 17, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR
BridgeAR requested review from jasnell, lundibundi, mcollina and targos January 20, 2020 11:16
mcollina
mcollina reviewed Jan 20, 2020
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@BridgeAR

BridgeAR commented Jan 20, 2020

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Jan 20, 2020
View reviewed changes
@BridgeAR BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 20, 2020
@nodejs-github-bot

nodejs-github-bot commented Jan 20, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lundibundi
lundibundi approved these changes Jan 20, 2020
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Jan 20, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 20, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
antsmartian
antsmartian approved these changes Jan 21, 2020
View reviewed changes
Comment thread lib/readline.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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

nodejs-github-bot commented Jan 22, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 25, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 26, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 26, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 26, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Jan 27, 2020
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Jan 27, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Feb 5, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 5, 2020
addaleax
addaleax approved these changes Feb 5, 2020
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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

nodejs-github-bot commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax

addaleax commented Feb 7, 2020

Copy link
Copy Markdown
Member

Landed in f9a27ea
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
v13.9.0 proposal #31837
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
readline: remove intermediate variable #31676
Merged
2 tasks done
@targos targos added the backport-blocked-v12.x label Apr 25, 2020
@targos

targos commented Apr 25, 2020

Copy link
Copy Markdown
Member

Depends on #31423 to land on v12.x
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added dont-land-on-v12.x and removed backport-blocked-v12.x labels Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina left review comments
@bnoordhuis bnoordhuis bnoordhuis left review comments
@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
@addaleax addaleax addaleax approved these changes
@targos targos Awaiting requested review from targos
+2 more reviewers
@lundibundi lundibundi lundibundi approved these changes
@antsmartian antsmartian antsmartian approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@BridgeAR @nodejs-github-bot @Trott @addaleax @targos @mcollina @bnoordhuis @jasnell @antsmartian @lundibundi
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.