Return all entries by default for iterator

[CSDUMMI]

I believe, that from my understanding of iterator and the Documentation of iterator,
the default limit should be -1, i.e. returning multiple items, instead of just one.

iterator implies multiple items, so it is counter intuitive to only return 1 Object by default..

[CSDUMMI]

This also makes a line more readable, I think,
by removing nested ternary operators.

https://github.com/CSDUMMI/orbit-db-eventstore/blob/c43f56515857058cec23e60e8fc8b5ddab9359f2/src/EventStore.js#L54

[tabcat]

this is a somewhat large change as it affects the api and should probably at the least cause a minor version update. i would rather this be a change that comes with the 1.0 release.

[aphelionz]

I'm fine with it being a minor version bump. We typically treat those like major versions anyway. From the semver spec:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

We just need to be really deliberate and explicit in the changelog

[CSDUMMI]

@tabcat you do support the idea of this change in general though?
Since you are already at version numbers and releasing.

[tabcat]

if the orbitdb code using the iterator is using something lower and passing the options directly to it, i would rather not override the defaults of the separate system. otherwise it seems fine, also db._oplog.values gives the same results as what you want to do here.

[CSDUMMI]

f the orbitdb code using the iterator is using something lower and passing the options directly to it, i would rather not override the defaults of the separate system.
@tabcat what do you mean by this?

[tabcat]

if the options and defaults are defined outside of the orbitdb codebase then i wouldnt want to override those defaults. luckily for you they are defined in the store and could easily be changed. i think it would be best to get the authors opinion on this change though since they may have set it to 1 by default for a reason.

[CSDUMMI]

@haadcode what do you think of this change?
I know that you might not recall, since the blame says, you set this default 5 years ago.

[CSDUMMI]

@tabcat I don't think there was any reason, that is still very valid, because it was set 5 years ago.

[aphelionz]

Not sure if the age of the code is any mark of its validity. Maybe let's just use semver here and classify this as a new major version?

[CSDUMMI]

Yes. I'm fine with that.

[tabcat]

what if opts.limit is -2

[aphelionz]

sounds like a good test case

[CSDUMMI]

Yeah. I doubt it would pass the test. Should we wrap it into a call to abs?

[aphelionz]

it should error out imo

[CSDUMMI]

@tabcat it should make no difference now, since I used Math.abs on the limit value.
@aphelionz a test case for this is a great idea though.
So I implemented that in the PR on orbit-db.

[CSDUMMI]

@aphelionz why? And if so, how?

[tabcat]

imo it shouldnt error, it should be the same as supplying a -1 as opt.limit similar to the old logic. the simplest, safest, and most obvious change to implement this would have been to just change the 1 at the end of the line to this._index.get().length.

[CSDUMMI]

Implementing it that way has the advantage of being more backwards compatible,
while @aphelionz's proposal is closer to the documentation. (Since it is not specified how other negative values, besides -1 should be handled.)

I would suggest third way, that has two advantages and one disadvantage at least:

• Instead of -1, 0 is used as the value to indicate all values should be returned and is used as a default.
• The limit, that isn't 0, is taken as defining the number of values to return.
• The sign of the limit determines, whether the elements should be taken from the front (+) or back of the iterator.

Example:

log.iterator({ limit: 0 }).collect() == [1,2,3,4,5]
log.iterator({ limit : -2 }).collect() == [4,5]
log.iterator({ limit: 2 }).collect() == [1,2]

The advantage of this is that this is similar to the negative indexing in, for example, Python.

But the disadvantage is of course, that it isn't very intuitive at first. (There'll probably be many people confusing it with the reverse option)

[CSDUMMI]

So, if you are interested in only getting the last five (in chronological order)
you provide the limit: -5, if you want the first five (in chronological order)
you provide the limit: 5.
Of course, the chronology reverses, with the reverse option.

[CSDUMMI]

Similarly if gte, gt, lt, lte is defined.
Then it will be 5 after or before the specified hash?
What do you think?