Skip to content
This repository was archived by the owner on Sep 30, 2023. It is now read-only.

Return all entries by default for iterator #38

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Return all entries by default for iterator #38

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ddb254
Return all entries by default for iterator
CSDUMMI Apr 30, 2021
c43f565
Update EventStore.js
CSDUMMI May 1, 2021
b64e7fa
Wrapping amount with Math.abs
CSDUMMI May 19, 2021
552365d
Allow negative values for amount
CSDUMMI May 19, 2021
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
3 changes: 2 additions & 1 deletion src/EventStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class EventStore extends Store {
get (hash) {
return this.iterator({ gte: hash, limit: 1 }).collect()[0]
}

iterator (options) {
const messages = this._query(options)
let currentIndex = 0
Expand All @@ -50,7 +51,7 @@ class EventStore extends Store {
_query (opts) {
if (!opts) opts = {}

const amount = opts.limit ? (opts.limit > -1 ? opts.limit : this._index.get().length) : 1 // Return 1 if no limit is provided
const amount = !opts.limit || opts.limit == -1 ? this._index.get().length : opts.limit; // Return all by default.
Copy link

Choose a reason for hiding this comment

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

what if opts.limit is -2

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds like a good test case

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

it should error out imo

Copy link
Author

@CSDUMMI CSDUMMI May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@aphelionz why? And if so, how?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

@CSDUMMI CSDUMMI May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

const events = this._index.get().slice()
let result = []

Expand Down