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

[Trott]

code fix, documentation, test

Fixes #1756

[Fishrock123]

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

[Trott]

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?

[Trott]

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]

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]

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]

This looks unnecessary.

[Trott]

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?

[silverwind]

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

[Trott]

Ah! OK, that makes sense.

[Trott]

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

[brendanashworth]

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

[Trott]

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

[Trott]

More commits pushed to include simplifications suggested by @brendanashworth

[Trott]

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

[Trott]

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]

@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]

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]

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]

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]

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

[Trott]

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.

[brendanashworth]

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

[brendanashworth]

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

[Trott]

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

[brendanashworth]

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

[Trott]

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

[brendanashworth]

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.

[Trott]

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

[brendanashworth]

That would perhaps work better, actually. Good catch.

[Trott]

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

[Trott]

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]

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

[thefourtheye]

Why do we need completer &&?

[Trott]

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 &&.

[thefourtheye]

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

[Trott]

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.

[thefourtheye]

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

[brendanashworth]

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

[thefourtheye]

Cool. Okay then 👍

[brendanashworth]

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

[Trott]

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]

@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]

Thank you, landed in 4b3d493.

[rvagg]

@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]

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

[Trott]

@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]

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

[Trott]

OK, re-relanded in 90c72d4