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.

Sign up for GitHub

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

Jump to bottom

"readability"fix(store): Clearer range check #227

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

"readability"fix(store): Clearer range check #227

wants to merge 2 commits into from

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Oct 23, 2024 •
edited
Loading

This PR cleans up a sanity check on the range request in the private method of store to make it more readable.

@renaynay renaynay self-assigned this Oct 23, 2024
@renaynay renaynay mentioned this pull request Oct 23, 2024
Merged
walldiss
walldiss previously approved these changes Oct 23, 2024
View reviewed changes
@cristaloleg
Copy link
Contributor

Can you add following lines to the TestStore in store/store_test.go ?

	out, err = store.getRangeByHeight(ctx, 10, 11)
	require.NoError(t, err)
	assert.Len(t, out, 1)

(at the end of the test, to prevent any regressions in the future)

cristaloleg
cristaloleg previously approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

LGTM but see comment regarding a few asserts

@renaynay renaynay dismissed stale reviews from cristaloleg and walldiss via 241f705 October 24, 2024 14:28
@renaynay renaynay changed the title fix(store): Allow a range of amount 1 to be requested from the store "readability"fix(store): Clearer range check Oct 24, 2024
cristaloleg
cristaloleg approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Wondertan
Wondertan reviewed Oct 24, 2024
View reviewed changes
store/store.go
// `from` with `to-1`
if from > to-1 {
return nil, fmt.Errorf("header/store: invalid range(%d,%d)", from, to-1)
if from >= to {
Copy link
Member

Choose a reason for hiding this comment

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

Did we check this on Exchange?

walldiss
walldiss approved these changes Oct 24, 2024
View reviewed changes
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

from > to-1 is the same as from >= to, but it's tricky to understand. The new version is much easier to follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan left review comments

@cristaloleg cristaloleg cristaloleg approved these changes

@walldiss walldiss walldiss approved these changes

@tzdybal tzdybal Awaiting requested review from tzdybal tzdybal is a code owner

@vgonkivs vgonkivs Awaiting requested review from vgonkivs vgonkivs is a code owner

@gupadhyaya gupadhyaya Awaiting requested review from gupadhyaya gupadhyaya is a code owner

Assignees

@renaynay renaynay

Labels
kind:fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@renaynay @cristaloleg @walldiss @Wondertan