repl: don't use tty control codes when $TERM is set to "dumb" by saljam · Pull Request #2712 · nodejs/node (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Issues and PRs related to the REPL subsystem.
label
@saljam sorry about this, but could rebase this PR once against current master?
Eh, I'm a little confused about what just happened. But sure, I just rebased. :)
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/
I think this is pretty low-risk, but could you add a test?
I can foresee a problem here. terminal controls a bunch of different things, and not just ANSI control codes.
@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.)
OK, I've switched to disabling useColor when TERM=dumb and added a basic test.
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.)
@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 pushed a commit that referenced this pull request
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 ;)
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).
@saljam it does include linting, and should've shown these errors after the test run.
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
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
Labels
Issues and PRs related to the REPL subsystem.