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: don't use tty control codes when $TERM is set to "dumb" #2712

Closed
wants to merge 1 commit into from
Closed

repl: don't use tty control codes when $TERM is set to "dumb" #2712

wants to merge 1 commit into from

Conversation

saljam
Copy link
Contributor

@saljam saljam commented Sep 5, 2015

This stops the REPL from using ANSI control codes on terminals
which don't support them. (Fixes nodejs/node-v0.x-archive#5344)

For history see nodejs/node-v0.x-archive#25506

@ChALkeR ChALkeR added the repl Issues and PRs related to the REPL subsystem. label Sep 5, 2015
@silverwind silverwind mentioned this pull request Sep 6, 2015
Closed
@silverwind
Copy link
Contributor

@saljam sorry about this, but could rebase this PR once against current master?

@saljam
Copy link
Contributor Author

saljam commented Sep 6, 2015

Eh, I'm a little confused about what just happened. But sure, I just rebased. :)

@silverwind
Copy link
Contributor

We had a happy little issue (#2713) which led us to force-pushing a few commits on top of master, and as your PR was based on one of the commits that was gone, it could no longer apply. The Github UI bugging out is another result of that.

Also, CI of this PR here: https://ci.nodejs.org/job/node-test-pull-request/258/

@silverwind
Copy link
Contributor

I think this is pretty low-risk, but could you add a test?

@Fishrock123
Copy link
Contributor

I can foresee a problem here. terminal controls a bunch of different things, and not just ANSI control codes.

@saljam
Copy link
Contributor Author

saljam commented Sep 13, 2015

@Fishrock123 as far as I can tell, it also disables readline and the up-down-arrow history.

Do we want readline on dumb terminals? If yes, I could just disable opts.useColors instead. (I have a vague memory of there being a reason I didn't do that, but that was over two years ago and I can't see why not now.)

@saljam
Copy link
Contributor Author

saljam commented Sep 13, 2015

OK, I've switched to disabling useColor when TERM=dumb and added a basic test.

@Fishrock123
Copy link
Contributor

LGTM. @saljam could you put a link into the commit log, and/or a comment in the code, to a reference on env.TERM? (i.e. if there is any sort of spec that defines it or something.)

@saljam
repl: don't use tty control codes when $TERM is set to "dumb"
c156277 
This change stops the REPL from using ANSI control codes for colours
when the TERM environment variable is set to "dumb".

"dumb" is the terminal type with the smallest set of capabilities as
described by terminfo.
http://invisible-island.net/ncurses/terminfo.ti.html#toc-_Specials

Fixes nodejs/node-v0.x-archive#5344
@saljam
Copy link
Contributor Author

saljam commented Sep 13, 2015

@Fishrock123 done. PTAL.

Git, hg, grep, &c all respect TERM=dumb, but surprisingly I couldn't find a clear definition other than in terminfo's source.

@silverwind
Copy link
Contributor

LGTM

@ChALkeR ChALkeR mentioned this pull request Sep 17, 2015
Closed
silverwind pushed a commit that referenced this pull request Sep 22, 2015
@saljam @silverwind
repl: don't use tty control codes when $TERM is set to "dumb"
ccea33d 
This change stops the REPL from using ANSI control codes for colours
when the TERM environment variable is set to "dumb".

"dumb" is the terminal type with the smallest set of capabilities as
described by terminfo. See:

http://invisible-island.net/ncurses/terminfo.ti.html#toc-_Specials

Related: nodejs/node-v0.x-archive#5344
Related: nodejs/node-v0.x-archive#25506
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
PR-URL: #2712
@silverwind
Copy link
Contributor

Landed with some fixups to the test in ccea33d.

I added a more helpful error message to the asserts and fixed the linter issues. @saljam please run make jslint next time before committing ;)

@silverwind silverwind closed this Sep 22, 2015
@saljam
Copy link
Contributor Author

saljam commented Sep 22, 2015

Thanks! And noted, somehow I convinced myself make test included a jslint
run.

On Tue, 22 Sep 2015 20:44 silverwind notifications@github.com wrote:

Landed with a few fixes to the test in ccea33d
ccea33d
.

I added a slightly more helpful error message to the asserts and fixed the
linter errors. @saljam https://github.com/saljam please run make jslint
next time before committing ;)

—
Reply to this email directly or view it on GitHub
#2712 (comment).

@silverwind
Copy link
Contributor

@saljam it does include linting, and should've shown these errors after the test run.

@saljam
Copy link
Contributor Author

saljam commented Sep 22, 2015

In which case whoops! Sorry! (I can confirm it does work.)

On Tue, 22 Sep 2015 20:52 silverwind notifications@github.com wrote:

@saljam https://github.com/saljam it does

node/Makefile

Lines 89 to 92 in ccea33d

test: | cctest # Depends on 'all'.
$(PYTHON) tools/test.py --mode=release message parallel sequential -J
$(MAKE) jslint
$(MAKE) cpplint

include linting, and should've shown these errors after the test run.

—
Reply to this email directly or view it on GitHub
#2712 (comment).

rvagg pushed a commit that referenced this pull request Sep 22, 2015
@saljam @rvagg
repl: don't use tty control codes when $TERM is set to "dumb"
9760e04 
This change stops the REPL from using ANSI control codes for colours
when the TERM environment variable is set to "dumb".

"dumb" is the terminal type with the smallest set of capabilities as
described by terminfo. See:

http://invisible-island.net/ncurses/terminfo.ti.html#toc-_Specials

Related: nodejs/node-v0.x-archive#5344
Related: nodejs/node-v0.x-archive#25506
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
PR-URL: #2712
@rvagg rvagg mentioned this pull request Sep 22, 2015
Merged
@j3pic j3pic mentioned this pull request Feb 18, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Apr 30, 2023
Open
@aliscco aliscco mentioned this pull request Apr 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.
4 participants
@saljam @silverwind @Fishrock123 @ChALkeR