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

IdsQueryBuilder: Allow to add a list in addition to array #11409

Conversation

@spinscale
Copy link
Member

commented May 29, 2015

In case a developer gets a List<String> of ids from another data source,
it does not make any sense, to convert it to an array first,
and then internally in IdsQueryBuilder a list is created a out of this again.

Closes #5089

@javanna

This comment has been minimized.

Copy link
Member

commented May 29, 2015

the only thing I don't like here is that we add two methods to do the same, but I see why we do it (consistency with existing methods I guess)... LGTM besides that

@spinscale spinscale added >enhancement and removed >non-issue labels May 29, 2015
@@ -54,11 +54,26 @@ public IdsQueryBuilder addIds(String... ids) {
/**
* Adds ids to the filter.
*/
public IdsQueryBuilder addIds(List<String> ids) {

This comment has been minimized.

Copy link
@javanna

javanna Jun 8, 2015

Member

minor nitpick: maybe List could be a Collection instead?

In case a developer gets a list of ids from another data source,
it does not make a lot of sense, to convert it to an array first,
and then internally in IdsQueryBuilder elasticsearch creates a
list out of this.

Closes #5089
@spinscale spinscale force-pushed the spinscale:1505-ids-querybuilder-allow-ids-as-list-issue-5089 branch from d3c1f63 to 57a94a1 Jun 9, 2015
@spinscale spinscale merged commit 57a94a1 into elastic:master Jun 9, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@kevinkluge kevinkluge removed the review label Jun 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.