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.

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

readline: Allow history to be disabled #6352

Closed
wants to merge 2 commits into from
Closed

readline: Allow history to be disabled #6352

wants to merge 2 commits into from

Conversation

@suryagh
Copy link
Contributor

@suryagh suryagh commented Apr 22, 2016

Checklist
  • tests and code linting passes
  • a test is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

readline

Description of change

Fixes #6336

  1. The historySize to default to 30 only if undefined.
  2. If historySize is set to 0, then disable caching the line.
  3. Added unit tests.
  4. Updated documentation.
@mscdex mscdex added the readline label Apr 22, 2016
@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 22, 2016

@suryagh May be the file reading example should also be updated by adding historySize: 0.

@suryagh
Copy link
Contributor Author

@suryagh suryagh commented Apr 23, 2016

@vsemozhetbyt,

Are you referring to modifying the Example: Read File Stream Line-by-Line to include historySize option? like:

const rl = readline.createInterface({
  input: fs.createReadStream('sample.txt'),
  historySize: 0
});

I would prefer not to worry about historySize = 0 vs the default 30. Unless someone has a strong opinion.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 23, 2016

@suryagh Yes, I referred this example and suggested this addition. Do you think this would be unnecessary?

I mean, this is almost a template code, many users could take it as a base for line by line file reading without going into details. This addition would spare them unnecessary overhead.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Apr 24, 2016

Sounds fine to me. Could someone describe the use-case in a bit more detail?

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 24, 2016

@Fishrock123 I often use readline to process big (up to 1 GB) text files (dictionaries) line by line: to reformat them, to extract data etc. For such processing history of lines is useless, it only wastes resources.

@cjihrig cjihrig added semver-major and removed semver-minor labels Apr 25, 2016
@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 25, 2016

Changing from semver-minor to semver-major. This changes behavior when historySize is zero.

@cjihrig
cjihrig reviewed Apr 25, 2016
View changes
test/parallel/test-readline-interface.js Outdated
fi = new FakeInput();
rli = new readline.Interface({ input: fi, output: fi, terminal: terminal,
historySize: 0 });
assert.equal(rli.historySize, 0);

This comment has been minimized.

@cjihrig

cjihrig Apr 25, 2016
Contributor

Please change this to assert.strictEqual().

This comment has been minimized.

@suryagh

suryagh Apr 25, 2016
Author Contributor

👍 Done.

@cjihrig
cjihrig reviewed Apr 25, 2016
View changes
test/parallel/test-readline-interface.js Outdated
// default history size 30
fi = new FakeInput();
rli = new readline.Interface({ input: fi, output: fi, terminal: terminal});
assert.equal(rli.historySize, 30);

This comment has been minimized.

@cjihrig

cjihrig Apr 25, 2016
Contributor

Please change this to assert.strictEqual() too.

This comment has been minimized.

@suryagh

suryagh Apr 25, 2016
Author Contributor

👍 Done.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 25, 2016

Can you add rli.close() after each of your tests.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 25, 2016

So now it can't be fixed untill Node 7.0, i.e. untill October 2016?

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 25, 2016

It could still potentially make it into v6.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 25, 2016

It would be nice to include a test that verifies that items are not being included in the history.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 25, 2016

Could anybody help if @suryagh could not implement all the requests untill v6 deadline?

@suryagh
Copy link
Contributor Author

@suryagh suryagh commented Apr 25, 2016

@vsemozhetbyt when is the v6 deadline?

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 25, 2016

I'll be cutting the v6.x branch tomorrow morning (likely around 9am pacific time) and will be kicking off the build sometime around 10am. Whatever lands before then should make it. I would prefer, however, to get any remaining semver-major PRs landed as soon as possible today in order to give more time for smoke and regression testing.

@suryagh suryagh force-pushed the suryagh:fix branch Apr 25, 2016
@addaleax
Copy link
Member

@addaleax addaleax commented Apr 25, 2016

LGTM

@suryagh Thanks for the changes!

@cjihrig
cjihrig reviewed Apr 25, 2016
View changes
doc/api/readline.md Outdated
@@ -300,7 +300,7 @@ the following values:
treated like a TTY, and have ANSI/VT100 escape codes written to it.
Defaults to checking `isTTY` on the `output` stream upon instantiation.

- `historySize` - maximum number of history lines retained. Defaults to `30`.
- `historySize` - maximum number of history lines retained. To disable the history set this value to `0`. Defaults to `30`.

This comment has been minimized.

@cjihrig

cjihrig Apr 25, 2016
Contributor

Can you wrap this at 80 characters.

This comment has been minimized.

@suryagh

suryagh Apr 25, 2016
Author Contributor

👍 Done.

@suryagh suryagh force-pushed the suryagh:fix branch Apr 25, 2016
@addaleax
Copy link
Member

@addaleax addaleax commented Apr 25, 2016

CI is green.

@suryagh The commit message may require a bit of cleanup, but we can do that upon landing if you prefer. Specifically:

  • The commit title should be all lowercase, I’d suggest readline: allow history to be disabled
  • You don’t need to include your bullet points 5 and 6, as they won’t make any sense outside the context of this PR.
  • The Author field is currently set to Panikkal, Surya(spanikkal). Is that intentional? Most people prefer to have their name in a more standard format (e.g. without your GitHub handle).
@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 25, 2016

LGTM, but I still think a test to show that the history is actually disabled would be nice.

@suryagh suryagh force-pushed the suryagh:fix branch Apr 25, 2016
@suryagh
Copy link
Contributor Author

@suryagh suryagh commented Apr 25, 2016

@cjihrig presently readline's history array is private to the module. Testing the private internal implementation detail sounds like a code smell to me. However, one approach to achieve this could be to expose the current number of the items in the history as a new api of readline, please let me know you think we should really take this route. Or, can you please point me to somewhere else in the code this is done in a more elegant fashion.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 25, 2016

There is nothing wrong with something testing its own internal implementation. There are a few private methods in readline you can call to manipulate history. You could also take a look at test/parallel/test-repl-persistent-history.js and see if anything there inspires you.

@suryagh suryagh force-pushed the suryagh:fix branch Apr 25, 2016
@suryagh
Copy link
Contributor Author

@suryagh suryagh commented Apr 25, 2016

@addaleax made the suggested changes.

fix #6336
1.  The `historySize` to default to `30` only if `undefined`.
2.  If `historySize` is set to 0, then disable caching the line.
3.  Added unit tests.
4.  Updated documentation.
@suryagh suryagh force-pushed the suryagh:fix branch to 5ac572c Apr 25, 2016
@addaleax
Copy link
Member

@addaleax addaleax commented Apr 25, 2016

@suryagh Cool! Could you look into @cjihrig’s suggestion to write an additional test for checking that this feature does what it’s supposed to do? As mentioned, it’s definitely okay to inspect private properties in the tests here.

Still LGTM either way.

@suryagh
Copy link
Contributor Author

@suryagh suryagh commented Apr 25, 2016

@addaleax I'm not sure if I can get to @cjihrig suggestion by tomorrow morning cut off time. If not, then I will send a separate PR for the readline tests. Please let me know if thats ok? I'm also thinking of making few additional fixes to the readline tests...unrelated to the current code changes.

Btw - regarding the github handle, it is actually suryagh. The spanikkal that is showing up in the commit is actually for our internal enterprise instance of the github..and for some reason git is using that for all pushes. I tried to ammend the commit to fix it, not sure if it is going through. Please feel free to change it to match other commit log.

@addaleax
Copy link
Member

@addaleax addaleax commented Apr 25, 2016

@suryagh PR’ed against your fork with a simple suggestion for tests. Be sure to write a small notification here if/when you update this PR so others can see that.

I have to admit I’d feel silly if this doesn’t get included in v6 over a missing test, esp. when it’s a breaking change anyway, so I’m adding it to the milestone. If someone thinks that that doesn’t leave enough time for reviews from more people, they should feel free to remove it.

@addaleax addaleax added this to the 6.0.0 milestone Apr 25, 2016
Check that when setting `historySize: 0`, the `history` property
of the readline interface is kept empty.
@suryagh
Copy link
Contributor Author

@suryagh suryagh commented Apr 25, 2016

@addaleax merged the tests. Thank you.

@addaleax
Copy link
Member

@addaleax addaleax commented Apr 25, 2016

Thanks! @cjihrig That does the trick for you?

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 25, 2016

LGTM. Thanks @addaleax

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 25, 2016

I've looked at the last tests of @addaleax and now I feel terribly silly with all the my 'overhead' alarm — the key reason I mention this bug in the first place by. It seems all the history caching does not even start if the terminal is false or undefined (in the functions and events chain _addHistory is not called at all).

All the same this bug fix is still a thing of consistency. However it probably makes sense to add in the readline.createInterface doc part a clarification that historySize has any meaning only if terminal becomes true by any way.

Sorry for all my stupidity and laziness to dig in this deeper (and for my poor English).

@addaleax
Copy link
Member

@addaleax addaleax commented Apr 25, 2016

@vsemozhetbyt Don’t worry about it too much. This is good for consistency, the less urgent it is the better, and you should by all means feel invited to write something up for the docs!

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 25, 2016

@addaleax Thank you. I'm not so familiar with all the colaboration cobweb, so may be I shoud not interfere in this now in the last fragile moment. But if this PR lands without such clarification, I shall try to add a simple doc PR for it when all the new release bustle ceases.

@addaleax
Copy link
Member

@addaleax addaleax commented Apr 25, 2016

@vsemozhetbyt I think this PR is more or less good to go, anything else can be done later.

@addaleax
Copy link
Member

@addaleax addaleax commented Apr 26, 2016

Landed in 0303a25.

@addaleax addaleax closed this Apr 26, 2016
addaleax added a commit that referenced this pull request Apr 26, 2016
1.  The `historySize` to default to `30` only if `undefined`.
2.  If `historySize` is set to 0, then disable caching the line.
3.  Added unit tests.
4.  Updated documentation.

Fixes: #6336
PR-URL: #6352
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell added a commit that referenced this pull request Apr 26, 2016
1.  The `historySize` to default to `30` only if `undefined`.
2.  If `historySize` is set to 0, then disable caching the line.
3.  Added unit tests.
4.  Updated documentation.

Fixes: #6336
PR-URL: #6352
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell added a commit that referenced this pull request May 1, 2016
History caching in the `readline` io is active only for terminal
interaction. Appropriate variables are initialized and relevant
`_addHistory()` function is called only if exposed `terminal` option
of `readline.createInterface()` is set `true` by user or internal
output check.

This clarification is useful to assure users there will be now wasted
overhead connected with history caching if `readline` is used not
for terminal interaction (e.g. for reading files line by line).

Particularly this fix is helpful after #6352 landing.

PR-URL: #6397
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexander Makarenko <estliberitas@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Fishrock123 added a commit that referenced this pull request May 4, 2016
History caching in the `readline` io is active only for terminal
interaction. Appropriate variables are initialized and relevant
`_addHistory()` function is called only if exposed `terminal` option
of `readline.createInterface()` is set `true` by user or internal
output check.

This clarification is useful to assure users there will be now wasted
overhead connected with history caching if `readline` is used not
for terminal interaction (e.g. for reading files line by line).

Particularly this fix is helpful after #6352 landing.

PR-URL: #6397
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexander Makarenko <estliberitas@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
joelostrowski added a commit to joelostrowski/node that referenced this pull request May 4, 2016
History caching in the `readline` io is active only for terminal
interaction. Appropriate variables are initialized and relevant
`_addHistory()` function is called only if exposed `terminal` option
of `readline.createInterface()` is set `true` by user or internal
output check.

This clarification is useful to assure users there will be now wasted
overhead connected with history caching if `readline` is used not
for terminal interaction (e.g. for reading files line by line).

Particularly this fix is helpful after nodejs#6352 landing.

PR-URL: nodejs#6397
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexander Makarenko <estliberitas@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants