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

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

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

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

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

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

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

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

It could still potentially make it into v6.

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

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

@vsemozhetbyt when is the v6 deadline?

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.

LGTM

@suryagh Thanks for the changes!

CI: https://ci.nodejs.org/job/node-test-commit/3048/

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

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

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

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.

@addaleax made the suggested changes.

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

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

@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 merged the tests. Thank you.

Thanks! @cjihrig That does the trick for you?

LGTM. Thanks @addaleax

One more CI: https://ci.nodejs.org/job/node-test-commit/3051/

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

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

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

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

Landed in 0303a25.