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

feat: Make Iterator implement Enumerable #45

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

AndrewSisley
Copy link
Collaborator

Relevant issue(s)

Resolves #41

Description

Makes Iterator implement Enumerable.

Avoids the direct reference from the production code, but enforces implementation in the tests.

Also simplifies the namespace code slightly, a lot got trimmed when making the initial change, and there may have been some extra stuff that could have been done outside of this PR.

@AndrewSisley AndrewSisley added this to the CoreKV v0.1 milestone Jan 29, 2025
@AndrewSisley AndrewSisley requested review from Lodek and a team January 29, 2025 23:47
@AndrewSisley AndrewSisley self-assigned this Jan 29, 2025
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

looks pretty good! thanks for cleaning up the namespace code too.

Only have one comment which is marked as a todo, but am approving anyways since its a quick/straight forward todo.

badger/badger.go Outdated
return false, err
}

if it.valid() && equal(it.i.Item().Key(), it.end) {
Copy link
Member

Choose a reason for hiding this comment

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

todo: Re-add the comment back as to why this section is necessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided against keeping the comment, as this if check is going very soon anyway in #27 as the start (end for reverse) are going to be inclusive.

Copy link
Collaborator Author

@AndrewSisley AndrewSisley Jan 30, 2025

Choose a reason for hiding this comment

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

Ah no, I got mixed up! This check is in the wrong place and valid/Seek are buggy.

  • Change something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the comment back, and expanded it. This line will be moved/removed when fixing badger Seek.

badger/badger.go Outdated
return false, err
}

if it.valid() && equal(it.i.Item().Key(), it.end) {
Copy link

@fredcarle fredcarle Jan 30, 2025

Choose a reason for hiding this comment

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

suggestion: The call to it.valid() is redundant as it.Seek() already calls it and that is what hasValue represents.

Copy link
Collaborator Author

@AndrewSisley AndrewSisley Jan 30, 2025

Choose a reason for hiding this comment

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

😁 I have a memory of removing that, maybe I reverted it along with a broken change, re-removing is problem free, thanks Fred.

  • Remove extra valid check

Comment on lines +83 to +84
// We don't need to bother loading the latest item in reverse, as the Last item
// will be of the latest version anyway.

Choose a reason for hiding this comment

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

praise: Thanks for this comment.

Comment on lines 94 to 95
// we can use unsafe here since we already aquired locks
// either prefix (priority) or start/end

Choose a reason for hiding this comment

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

todo: This comment can be removed as we no longer use unsafe.

Copy link
Collaborator Author

@AndrewSisley AndrewSisley Jan 30, 2025

Choose a reason for hiding this comment

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

I don't think we used unsafe at the time we removed the mutexes anyway :)

Nice spot, thanks Fred.

  • Remove outdated comment

Copy link

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM. Let's just resolve the comments before merge :)

@AndrewSisley AndrewSisley merged commit fc82eb6 into sourcenetwork:main Jan 30, 2025
4 checks passed
@AndrewSisley AndrewSisley deleted the 41-enumerable-iter branch January 30, 2025 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have Iterator implement Enumerable
3 participants