readline: enable tabs for input if tab completion is not enabled, fixes #1756 #1761

Trott · 2015-05-22T03:36:48Z

code fix, documentation, test

Fishrock123 · 2015-05-22T04:52:30Z

lib/readline.js

(typeof completer !== 'function || typeof completer !== 'undefined') will probably be inlined better by v8 / JS vm's.

Heh. Guess I should add a test for this line because mine doesn't work at all. Should be using indexOf() for test against the Array rather than in. Does the inline/optimization advice still apply if I'm doing ['function', 'undefined'].indexOf(typeof completer) === -1?

I just ran my own benchmark (which, hopefully, I didn't mess up) and (typeof completer !== 'function' && typeof completer !== 'undefined') is more than three times as fast as (['function', 'undefined'].indexOf(typeof completer) === -1). So I guess I need to (a) add a test and (b) use your suggestion.

Trott · 2015-05-22T07:07:04Z

Pushed new changes based on helpful feedback from @Fishrock123.

More tests
Refactored check for invalid completer option so that it is more V8-optimizer friendly and (more importantly) actually works.

Trott · 2015-05-22T07:13:12Z

Annnndddd.... pushed one more commit with some small changes because I forgot to run make jslint before my last commit. All tests still pass etc. etc.

silverwind · 2015-05-22T17:02:27Z

lib/readline.js

This looks unnecessary.

So far as I know, I didn't touch those lines or any code that uses those values. If they were unused, I would have thought the linter would have caught it. Maybe it doesn't lint on const values?

I meant the line removal, double newline is pretty standard for us.

Ah! OK, that makes sense.

Trott · 2015-05-22T18:39:36Z

Based on feedback from @silverwind, I pushed another commit restoring a newline that I removed from the source code unnecessarily.

brendanashworth · 2015-05-22T23:36:00Z

doc/api/readline.markdown

I don't think it is necessary to specify that they'll be permitted - do you think it can be inferred from above?

I think you're right and barring contrary opinion from someone else, I'll remove that second sentence.

Trott · 2015-05-23T01:30:22Z

More commits pushed to include simplifications suggested by @brendanashworth

Trott · 2015-05-24T22:04:31Z

One more commit for refactoring + lint comment per @Fishrock123 and @brendanashworth

Trott · 2015-05-26T04:29:08Z

So, now that this is sitting here and I'm waiting for either more feedback or for it to hopefully be merged, I'm naturally second-guessing the whole thing.

The primary concern I have is the end user annoyance factor: Programs often don't give an obvious indication as to whether tab completion is enabled or not. Users might typically press the tab key to find out. If it shows options, it's enabled. If it does nothing, carry on. We're altering that so that a user that hits tab now has to delete the character.

Am I overthinking it? Is this truly a bug with some real value in fixing?

If my concerns are valid, the pull request is still salvageable.

It would just be reduced to a test or two, and the deleting of the comment that indicates there is a bug to be fixed.

brendanashworth · 2015-05-26T05:05:08Z

@Trott I think you're overthinking it (p.s. I'll be reviewing within the coming week, just busy rn). If there isn't tab completion available, to me, it makes sense to print out a tab (the user did press the key!) rather than eat it silently. Plus, if a user wants to indent inside a REPL, why force them to use spaces?

Trott · 2015-05-26T05:22:42Z

OK, good enough for me, I'm convinced. (And, just in case it came off wrong, I'm not complaining that this is taking a long time or anything like that. You people are awesome.)

brendanashworth · 2015-05-30T08:17:14Z

test/parallel/test-readline-interface.js

Using an array with expectedLines is probably unnecessary, you could just assert.equal(line, '\t') in the callback and use a truthy/falsy called.

Trott · 2015-05-30T15:10:31Z

I pushed two more commits based on @brendanashworth feedback. One simplifies test code, the other removes a statement of the obvious (that you can't input a tab if tab completion is enabled) from the doc.

brendanashworth · 2015-05-31T21:45:22Z

test/parallel/test-readline-interface.js

Could you assert.strictEqual(called, false) here and below?

Probably a good idea to change all my instances of assert.ok() and assert.equal() to .strictEqual() since I know exactly what the value and type should be, so I'll go ahead and do that. Stop me if that's a terrible idea for some reason I'm not considering.

Not a terrible idea, just wouldn't really make a difference for most.

brendanashworth · 2015-05-31T21:56:43Z

Once you make your changes, could you rebase&squash and write out the commit so it fits this?

Trott · 2015-05-31T22:12:07Z

@brendanashworth assert.strictEqual() is now in there, commits are squashed to a single commit, commit message now conforms to CONTRIBUTING.md guidelines.

brendanashworth · 2015-06-01T05:52:20Z

lib/readline.js

Am I right in thinking that you can keep the code the way it was before by changing strict equality to loose equality? It will work the same either way, but keep the code fresher.

Edit: there is also a trailing whitespace character here

Can you clarify what you mean by this? I'm not sure what "the way it was before" means here.

Sure, sorry. Since the line was 79 characters before and the indentation adds 2, it is now 81. If you change the === to ==, we can keep it at 80 and remove a line. Like this:

- this.completer = completer.length === 2 ? completer : function(v, callback) { - callback(null, completer(v)); - }; + if (typeof completer === 'function') { + this.completer = completer.length == 2 ? completer : function(v, callback) { + callback(null, completer(v)); + }; + }

The === and == change shouldn't make a difference, since .length will always be a number.

Ah! I see. Alternatively, I can abbreviate callback as cb, if that's not objectionable.

That would perhaps work better, actually. Good catch.

Trott · 2015-06-02T02:52:00Z

Pushed a new version to keep the ternary all on a single line as suggested by @brendanashworth

Trott · 2015-06-02T02:54:25Z

I think whether it's a bugfix or a new feature would probably depends on who you ask. I suspect it's a better idea to err on the side of caution and go with semver-minor. But I don't feel strongly about it. I have lots of wrong opinions, and this might be one of them.

brendanashworth · 2015-06-03T03:11:48Z

Sweet! LGTM. I put it back as semver-minor, and will leave this for a day if anyone else wants to review.

thefourtheye · 2015-06-03T14:04:49Z

lib/readline.js

Why do we need completer &&?

If no completer was explicitly provided, it will be undefined here. So valid values at this point are undefined (or more precisely, the way the code is, falsy values) or a function. It's only a TypeError if a completer was provided and it is not a function. So completer &&.

Okay, if it is a falsy value then its not a typeerror in this case?

That's my feeling. If you send it false (for example) then you are probably trying to say "Don't use a completer." In the spirit of being liberal with what is accepted but conservative with what is emitted, I wrote it up to allow falsy values. But strict checking for undefined would work too.

Let's wait for others to take a look at this. I feel that we should allow only if it is a function.

By allowing falsy values, we keep backwards compatibility. They were accepted before this.

Cool. Okay then 👍

brendanashworth · 2015-06-03T16:53:04Z

just in case: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/761/

Trott · 2015-06-03T19:00:26Z

I'm not familiar with io.js Jenkins setup. What's up with the arm7-wheezy test failure? Seems unrelated to these changes, but then again, that's why there are tests. Do some tests or platforms have intermittent test failures and you just re-run them to see if it gets unstuck?

brendanashworth · 2015-06-03T19:05:41Z

@Trott that was a mostly successful run. That failure seems to be unrelated (although I've never seen it before, and I'll probably open an issue for it. It looks ugly.) There are a few known failures for the weaker platforms, but we don't usually rerun them.

edit: see #1888

brendanashworth · 2015-06-06T20:35:25Z

Thank you, landed in 4b3d493.

rvagg · 2015-06-12T00:24:01Z

@nodejs/collaborators we need some help determining if this is semver-minor or semver-major. If it's the latter then we'll revert off master and move it in to next as we're going to have a 2.3.0 tomorrow.

Gleaned from discussion over in #1939:

@Trott:

It changes swallowing tabs in readline if there's no tab-completion functionality enabled behavior to tabs are now permitted in the user input if there's no tab-completion functionality enabled.

The old behavior wasn't documented (as far as I know), but it does seem like it was a choice and by design.

And it seems at least conceivable that it is relied upon by some people in a way that will break things for them. (And they may see the existing behavior as a feature and not a bug.) Something like "My code took the user input and stripped out all the space characters but now it comes in with tabs and so there's whitespace and when I pass the string to my database call it blows up." Granted, that's a scenario where someone is Doing It Wrong, but a breaking change is a breaking change.

@brendanashworth:

AFAIK tabs were the only character not accepted by readline, which was caused by an incorrectly implemented feature that made all tabs disappear, regardless of whether or not the feature was on. It was marked as BUG in the code - but swallowing tabs isn't really something someone should rely on, or would imho. It definitely wasn't intentional.

Thanks for bringing this question up @Trott

Fishrock123 · 2015-06-12T22:59:00Z

Re-landed on next at 2ebc77d as per #1961

Trott · 2015-06-22T18:48:18Z

@Fishrock123 This doesn't look like it was re-landed in next. (Or it was re-landed and something force-pushed over it?) Any objection to me re-landing it myself?

trevnorris · 2015-06-22T18:54:02Z

Looks like that commit may have been overridden on a rebase of next. Feel free to land it again.

Trott · 2015-06-22T19:05:18Z

OK, re-relanded in 90c72d4

Trott mentioned this pull request May 22, 2015

readline: tabs not accepted is a bug or a feature? #1756

Closed

mscdex added the readline Issues and PRs related to the built-in readline module. label May 22, 2015

Fishrock123 reviewed May 22, 2015
View reviewed changes

silverwind reviewed May 22, 2015
View reviewed changes

Trott force-pushed the enable-tab branch from 00c0e12 to 2296b86 Compare May 22, 2015 18:37

brendanashworth reviewed May 22, 2015
View reviewed changes

Trott force-pushed the enable-tab branch from a14de9f to 75ec0a4 Compare May 23, 2015 00:07

Trott force-pushed the enable-tab branch 2 times, most recently from facc281 to f105fde Compare May 24, 2015 21:19

Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label May 26, 2015

brendanashworth reviewed May 30, 2015
View reviewed changes

Trott force-pushed the enable-tab branch from b2774b3 to f92f9c3 Compare May 30, 2015 15:00

brendanashworth reviewed May 31, 2015
View reviewed changes

Trott force-pushed the enable-tab branch from 98a48b2 to 40e58b8 Compare May 31, 2015 22:08

brendanashworth reviewed Jun 1, 2015
View reviewed changes

Trott force-pushed the enable-tab branch from 3d27eee to 285751a Compare June 2, 2015 02:51

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

brendanashworth added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 3, 2015

thefourtheye reviewed Jun 3, 2015
View reviewed changes

brendanashworth closed this Jun 6, 2015

rvagg mentioned this pull request Jun 11, 2015

Release proposal: 2.3.0 #1939

Closed

rvagg mentioned this pull request Aug 3, 2015

Release proposal: v3.0.0 #2221

Closed

rvagg mentioned this pull request Aug 4, 2015

Release proposal: v3.0.0 #2299

Merged

Trott deleted the enable-tab branch October 14, 2021 13:47

Uh oh!

readline: enable tabs for input if tab completion is not enabled, fixes #1756 #1761

readline: enable tabs for input if tab completion is not enabled, fixes #1756 #1761

Uh oh!

Conversation

Trott commented May 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented May 22, 2015

Uh oh!

Trott commented May 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented May 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented May 23, 2015

Uh oh!

Trott commented May 24, 2015

Uh oh!

Trott commented May 26, 2015

Uh oh!

brendanashworth commented May 26, 2015

Uh oh!

Trott commented May 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented May 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendanashworth commented May 31, 2015

Uh oh!

Trott commented May 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 2, 2015

Uh oh!

Trott commented Jun 2, 2015

Uh oh!

brendanashworth commented Jun 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment