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

repl: revert multiline history #24804

Closed
wants to merge 2 commits into from
Closed

repl: revert multiline history #24804

wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 3, 2018

So far the multiline history caused a lot of problems and the current logic is not suitable at all as such. It changes the actual content of the history and should not be used at all.

I struggle a lot with my history at the moment as it often forgets entries and similar and until a proper solution is found this should be reverted.

We discussed some possibilities how to properly implement this feature in #24231.

Refs: #24231

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

@BridgeAR BridgeAR requested a review from antsmartian December 3, 2018 13:47
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1801/pipeline

@nodejs-github-bot nodejs-github-bot added readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Dec 3, 2018
@BridgeAR BridgeAR changed the title Revert multiline history repl: revert multiline history Dec 3, 2018
@antsmartian
Copy link
Contributor

@BridgeAR Agree with you. We can revisit this idea with the ideas suggested on the issue tracker.

@antsmartian
Copy link
Contributor

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

@BridgeAR BridgeAR requested a review from targos December 3, 2018 17:09
@addaleax
Copy link
Member

addaleax commented Dec 3, 2018

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 3, 2018
@targos targos mentioned this pull request Dec 4, 2018
Closed
@BridgeAR BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 4, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 4, 2018

Please +1 to fast track this.

@addaleax
Copy link
Member

addaleax commented Dec 4, 2018

This doesn’t really seem suitable for fast-tracking? Is there a specific reason for doing so?

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 4, 2018

@addaleax I want to prepare the next 11.x release and I would like to include this PR in that release. Since the feature is clearly broken (there were multiple bug reports about it), I don't see why this should not be suitable?

@addaleax
Copy link
Member

addaleax commented Dec 4, 2018

@BridgeAR I trust you to make that call if you are sure about it, but generally, removing methods from a class that is public API is not exactly what we usually fast-track? If this is really broken, then yeah, I guess you can go ahead 👍

@joyeecheung joyeecheung mentioned this pull request Dec 5, 2018
Closed
@addaleax
Copy link
Member

addaleax commented Dec 5, 2018

@BridgeAR Looks like this needs a rebase?

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 5, 2018
@BridgeAR BridgeAR force-pushed the fix-multiline-history branch from 746b235 to 1a33e14 Compare December 5, 2018 17:25
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 5, 2018

Rebased

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

@danbev
Copy link
Contributor

danbev commented Dec 6, 2018 •
edited
Loading

Re-run of failing node-test-commit-windows-fanned ✔️

@Trott
Copy link
Member

Trott commented Dec 6, 2018

Landed in aa943d0...b1ada6c

@Trott Trott closed this Dec 6, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 6, 2018
@BridgeAR @Trott
Revert "repl: handle buffered string logic on finish"
fd2467a 
This reverts commit eb42c1e.

PR-URL: nodejs#24804
Refs: nodejs#24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 6, 2018
@BridgeAR @Trott
Revert "lib: repl multiline history support"
b1ada6c 
This reverts commit dd7a3d2.

PR-URL: nodejs#24804
Refs: nodejs#24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR added a commit that referenced this pull request Dec 6, 2018
@BridgeAR
Revert "repl: handle buffered string logic on finish"
858b23a 
This reverts commit eb42c1e.

PR-URL: #24804
Refs: #24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR added a commit that referenced this pull request Dec 6, 2018
@BridgeAR
Revert "lib: repl multiline history support"
3c98cf1 
This reverts commit dd7a3d2.

PR-URL: #24804
Refs: #24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR added a commit that referenced this pull request Dec 6, 2018
@BridgeAR
2018-12-06, Version 11.4.0 (Current)
758cb07 
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
@BridgeAR BridgeAR mentioned this pull request Dec 6, 2018
Merged
4 tasks
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
Revert "repl: handle buffered string logic on finish"
6fed6f5 
This reverts commit eb42c1e.

PR-URL: #24804
Refs: #24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
Revert "lib: repl multiline history support"
73bc5fd 
This reverts commit dd7a3d2.

PR-URL: #24804
Refs: #24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
Revert "repl: handle buffered string logic on finish"
adae559 
This reverts commit eb42c1e.

PR-URL: #24804
Refs: #24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
Revert "lib: repl multiline history support"
f1bcf90 
This reverts commit dd7a3d2.

PR-URL: #24804
Refs: #24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
2018-12-07, Version 11.4.0 (Current)
4f1297f 
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
2018-12-07, Version 11.4.0 (Current)
bcef949 
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
Revert "repl: handle buffered string logic on finish"
1e656f5 
This reverts commit eb42c1e.

PR-URL: nodejs#24804
Refs: nodejs#24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
Revert "lib: repl multiline history support"
f1af194 
This reverts commit dd7a3d2.

PR-URL: nodejs#24804
Refs: nodejs#24231
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
2018-12-07, Version 11.4.0 (Current)
5912f6d 
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    nodejs#23708
  * The inspection `depth` default is now back at 2.
    nodejs#24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    nodejs#23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    nodejs#24739
* readline:
  * The `readline` module now supports async iterators.
    nodejs#23916
* repl:
  * The multiline history feature is removed.
    nodejs#24804
* tls:
  * Added min/max protocol version options.
    nodejs#24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. nodejs#24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    nodejs#23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    nodejs#24677
  * The install-tools scripts or now included in the dist.
    nodejs#24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    nodejs#24655

PR-URL: nodejs#24854
@aleclarson aleclarson mentioned this pull request Jan 21, 2019
Closed
@BridgeAR BridgeAR deleted the fix-multiline-history branch January 20, 2020 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@antsmartian antsmartian antsmartian approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@BridgeAR @nodejs-github-bot @antsmartian @addaleax @danbev @Trott @Fishrock123 @targos @BethGriggs