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

Uid#createTypeUids to accept a collection of ids rather than a list #11263

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented May 20, 2015

The downside of having createTypeUids accept a list only is that if you do provide a collection nothing breaks at compile time, but you end up calling the same method that accepts an object as second argument. Renamed both methods to avoid clashes to createUidsForTypesAndId and createUidsForTypesAndIds. The latter accepts now a Collection of Objects rather than just a List.

@jpountz
Copy link
Contributor

jpountz commented May 20, 2015

@javanna Maybe we could also rename createTypeUids to createTypeUid (removing the trailing 's') when there is a single provided ID, this would avoid having again the same issue with eg. iterables?

@javanna
Copy link
Member Author

javanna commented May 21, 2015

@jpountz indeed that is what I had done at first, but the plural makes sense because the uids generated are still more than one although the id provided as argument is only one (number of types * number of ids). Shall we still do it?

@jpountz
Copy link
Contributor

jpountz commented May 21, 2015

Maybe another way to see it is that this method should not really take Object's. Since all our ids are supposed to be strings, I think onlyStringorBytesRef` could really make sense?

@javanna
Copy link
Member Author

javanna commented May 21, 2015

Maybe another way to see it is that this method should not really take Object's. Since all our ids are supposed to be strings, I think onlyStringorBytesRef` could really make sense?

I tend to agree, but this would be a bigger change, that has to do with these methods being called from FieldMapper#termQuery & FieldMapper#termsQuery. I went for renaming both methods, hopefully that makes it clearer.

@jpountz
Copy link
Contributor

jpountz commented May 21, 2015

LGTM

…n a list, plus rename method variants to avoid clashes

The downside of having createTypeUids accept a list only is that if you do provide a collection nothing breaks at compile time, but you end up calling the same method that accepts an object as second argument. Renamed both methods to avoid clashes to `createUidsForTypesAndId` and `createUidsForTypesAndIds`. The latter accepts now a Collection of Objects rather than just a List.

Closes elastic#11263
@javanna javanna force-pushed the enhancement/uid_createTypeUids_collection_ids branch from b56c109 to 5a0c456 Compare May 21, 2015 08:43
@javanna javanna removed the review label May 21, 2015
@javanna javanna merged commit 5a0c456 into elastic:master May 21, 2015
@javanna javanna removed the v1.6.0 label May 21, 2015
@clintongormley clintongormley changed the title Internal: Uid#createTypeUids to accept a collection of ids rather than a list Uid#createTypeUids to accept a collection of ids rather than a list Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants