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

change Selector#value() to respect Selector#offset() #405

Merged
merged 1 commit into from
May 18, 2017

Conversation

k-kagurazaka
Copy link
Member

Currently, Selector#value() and Selector#valueOrNull() returns the first model whether or not to specify Selector#offset().

orma.selectFromTodo()
    .offset(10)
    .value(); // returns the 1st todo without any warning!

Of course we can do the aimed thing as

orma.selectFromTodo().get(10);

but current behavior makes me a little confusable.

So I changed these methods to respects offset.
How do you think?

final long localOffset = offset;
if (localOffset > 0) {
return getOrNull(localOffset);
}
return getOrNull(0);
Copy link
Member

Choose a reason for hiding this comment

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

I think it is getOrNull() that should respect offset.

BTW I preffer Math.max(offset, 0) because it is simpler than if statements.

@k-kagurazaka
Copy link
Member Author

I pushed a commit based on your advice.

These commits look little messy.
I can squash & push if you like.

@gfx
Copy link
Member

gfx commented May 18, 2017

I think offset(i).get(0) and offset(i).getOrNull(0) also respect offset.

That is, value() should alyways the same as get(0).

Use offset in get() and getOrNull() instead of value().

@k-kagurazaka
Copy link
Member Author

You mean offset(5).get(2) return 8th element, right?

@gfx
Copy link
Member

gfx commented May 18, 2017

Yep. It seems more natural, isn't it?

@k-kagurazaka
Copy link
Member Author

All done!
Finally, code changes are quite simple, so I squashed these commits 😃

@gfx
Copy link
Member

gfx commented May 18, 2017

LGTM. Thanks for your contribution 👏

@gfx gfx merged commit 9dec162 into maskarade:master May 18, 2017
@k-kagurazaka k-kagurazaka deleted the value-respects-offset branch May 18, 2017 10:07
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.

2 participants