Skip to content

readline: remove max limit of crlfDelay #13497

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

Closed
Azard wants to merge 8 commits into from
Closed

readline: remove max limit of crlfDelay #13497

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e1038fa
readline: remove max limit of crlfDelay
Azard Jun 6, 2017
056999e
doc: api/readline.md clearer
Azard Jun 6, 2017
b97c9c4
readline: set crlfDelay to Infinity is allowed
Azard Jun 6, 2017
f786414
doc: clearer
Azard Jun 7, 2017
43b78ec
update doc
Azard Jul 30, 2017
d7c1873
wrap line around 80 chars
Azard Jul 30, 2017
d90647d
fix ident lint error
Azard Jul 30, 2017
3d9c7a9
crlfDelay is number, not convert to int
Azard Jul 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
readline: set crlfDelay to Infinity is allowed
  • Loading branch information
@Azard
Azard committed Jul 30, 2017
commit b97c9c41077bdf1a9170097acf9074ad2a930ee5
2 changes: 1 addition & 1 deletion doc/api/readline.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ changes:
* `crlfDelay` {number} If the delay between `\r` and `\n` exceeds
`crlfDelay` milliseconds, both `\r` and `\n` will be treated as separate
end-of-line input. Default to `100` milliseconds.
`crlfDelay` will be coerced to `100` or greater.
`crlfDelay` will be coerced to `100` or greater, `Infinity` is allowed.
* `removeHistoryDuplicates` {boolean} If `true`, when a new input line added
to the history list duplicates an older one, this removes the older line
from the list. Defaults to `false`.
Expand Down
3 changes: 2 additions & 1 deletion lib/readline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ function Interface(input, output, completer, terminal) {
this.input = input;
this.historySize = historySize;
this.removeHistoryDuplicates = !!removeHistoryDuplicates;
this.crlfDelay = Math.max(kMincrlfDelay, crlfDelay >>> 0);
Copy link
Copy Markdown
Contributor Author

@Azard Azard Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infinity >>> 0 is 0 😢

Copy link
Copy Markdown
Contributor

@Fishrock123 Fishrock123 Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you fixed this?

Copy link
Copy Markdown
Contributor Author

@Azard Azard Jun 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I fixed it

this.crlfDelay = crlfDelay === Infinity ?
Copy link
Copy Markdown
Contributor

@refack refack Jul 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, could just collapse to Math.max(kMincrlfDelay, crlfDelay);

Copy link
Copy Markdown
Contributor Author

@Azard Azard Jul 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack This is because crlfDelay is number type (not integer), crlfDelay >>> 0 convert float to integer, but Infinity >>> 0 is 0.

Copy link
Copy Markdown
Contributor

@refack refack Jul 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

being an integer might have been more important at some point in the past, but if you look at the current uses:
https://github.com/Azard/node/blob/d90647d0952091bd7053404945f1e01c57f5e98c/lib/readline.js#L412
and
https://github.com/Azard/node/blob/d90647d0952091bd7053404945f1e01c57f5e98c/lib/readline.js#L925
AFAICT treating it as just a number should work fine... (just need to make sure it's not passed to C++ and cast to int).

Copy link
Copy Markdown
Contributor Author

@Azard Azard Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this with
this.crlfDelay = crlfDelay ? Math.max(kMincrlfDelay, crlfDelay) : kMincrlfDelay;
to make sure crlfDelay is not undefined, and add one unit test to check this.crlfDelay can be float number.
crlfDelay is a pure JavaScript variable, won't pass to C++.

Copy link
Copy Markdown
Contributor Author

@Azard Azard Jul 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack Could you help me to start CI ?

Infinity : Math.max(kMincrlfDelay, crlfDelay >>> 0);

// Check arity, 2 - for async, 1 for sync
if (typeof completer === 'function') {
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-readline-interface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,29 @@ function isWarned(emitter) {
}), delay);
}

// set crlfDelay to `Infinity` is allowed
{
const fi = new FakeInput();
const delay = 200;
const crlfDelay = Infinity;
const rli = new readline.Interface({
input: fi,
output: fi,
terminal: terminal,
crlfDelay
});
let callCount = 0;
rli.on('line', function(line) {
callCount++;
});
fi.emit('data', '\r');
setTimeout(common.mustCall(() => {
fi.emit('data', '\n');
assert.strictEqual(callCount, 1);
rli.close();
}), delay);
}

// \t when there is no completer function should behave like an ordinary
// character
fi = new FakeInput();
Expand Down