Skip to content

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Aug 4, 2016

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

repl

Description of change

Disable Windows support for interrupting REPL commands using Ctrl+C by default, because the switches from console raw mode and back have been interfering with printing the results of
evaluated expressions.

This is a temporary measure, since the underlying problem is very likely not related to this specific feature.

Ref: #7837

/cc @joaocgreis

Disable Windows support for interrupting REPL commands using
Ctrl+C by default, because the switches from console raw mode
and back have been interfering with printing the results of
evaluated expressions.

This is a temporary measure, since the underlying problem is
very likely not related to this specific feature.

Ref: nodejs#7837
@addaleax addaleax added repl Issues and PRs related to the REPL subsystem. windows Issues and PRs related to the Windows platform. labels Aug 4, 2016
@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

Oy, this is unfortunate but LGTM as a temporary measure.

/cc @nodejs/platform-windows

@seishun
Copy link
Contributor

seishun commented Aug 4, 2016

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Aug 4, 2016

LGTM

@joaocgreis
Copy link
Member

LGTM, thanks for the env var!

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

oh! mentioning the env var reminded me... is the intent to keep the env var undocumented and temporary as well? If not, then this should also add documentation to printHelp and docs.

@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

@jasnell No, keeping it uncodumented is exactly the plan (or at least what I am having in mind). I thought about getting fancy with underscores or something, but that just seemed weird for env vars.

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

+1 awesome. That's what I suspected but wanted to clarify. It may be worthwhile adding a code comment to that effect in case someone else comes across it and wonders.

@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

@jasnell Done! :)

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

Thank you! :-)

@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

New CI run just to be safe: https://ci.nodejs.org/job/node-test-pull-request/3571/

jasnell pushed a commit that referenced this pull request Aug 8, 2016
Disable Windows support for interrupting REPL commands using
Ctrl+C by default, because the switches from console raw mode
and back have been interfering with printing the results of
evaluated expressions.

This is a temporary measure, since the underlying problem is
very likely not related to this specific feature.

Ref: #7837

PR-URL: #7977
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

Landed in f59b888!

@jasnell jasnell closed this Aug 8, 2016
@addaleax addaleax deleted the disable-ctrlc-windows branch August 8, 2016 16:45
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Disable Windows support for interrupting REPL commands using
Ctrl+C by default, because the switches from console raw mode
and back have been interfering with printing the results of
evaluated expressions.

This is a temporary measure, since the underlying problem is
very likely not related to this specific feature.

Ref: #7837

PR-URL: #7977
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@MylesBorins
Copy link
Contributor

@addaleax does this need to be backported?

@addaleax
Copy link
Member Author

No. :)

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. windows Issues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants