Add search by labels. #284
Conversation
Differentiate find with boolean AND with OR.
|
@dastultz Could you try to add some integration tests please ? |
|
Sorry for the delay, I plan to do this, please don't close the issue. Thanks. |
Unit tests.
dastultz
requested review from
aheritier,
duemir,
johnou and
stephenc
as code owners
|
Added unit tests. I did not run them! Documented article contents needed in sandbox. |
Fix typo in assertion.
| .set("query", searchTerm).set("section", sectionId), handleList(Article.class, "results")); | ||
| } | ||
|
|
||
| public Iterable<Article> getArticleFromSomeLabels(List<String> labels) { |
There was a problem hiding this comment.
Naming seems a little off to me, especially with the fact singular is used for returning a collection. How about something like..
getArticlesWithAllLabels
searchArticlesWithLabels
There was a problem hiding this comment.
I don't understand what is meant by "get" vs "search" if anything. The part I was looking to make clear was that one method requires at least one label to be found, the other requires all labels to be found. IMO, "WithAll" is good, but the idea of "with at least one" is not captured in the name "searchArticlesWithLabels". Not sure if "search" implies such behavior. You objected to the use of singular for returning a collection, I agree with that, I followed the existing pattern established by the getArticleFromSearch method. I think getArticlesFromSomeLabels and getArticlesFromAllLabels best captures the concerns, though I don't like the apparent API inconsistency with the singular Article vs Articles.
There was a problem hiding this comment.
How about getArticlesFromAllLabels and getArticlesFromAnyLabels
There was a problem hiding this comment.
Works for me, as long as everyone doesn't mind the inconsistency of Article plurality with getArticleFromSearch. Should I make the change, or will someone else do it?
There was a problem hiding this comment.
Or introduce corrected method with deprecated annotation on the singular.
There was a problem hiding this comment.
Who is the "gatekeeper" of the public-facing API? I'm trying to add functionality but not dictate API design in general. I feel like someone more qualified should handle the naming issue. I'm happy with getArticlesFromAllLabels and getArticlesFromAnyLabels. I'm going to make that change and hope it goes through.
Rename API methods as per code review.
4 participants
No description provided.