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

Added /search/web/first API endpoint #711

Merged
merged 1 commit into from Jul 10, 2017
Merged

Conversation

mickeyn
Copy link
Contributor

@mickeyn mickeyn commented Jul 5, 2017

This will simplify the reading of /search/web/simple output by
extracting the first element and eliminating the need for WEB
to read an ES output structure.

@mickeyn mickeyn requested a review from oalders July 5, 2017 08:09
oalders
oalders previously requested changes Jul 5, 2017
@@ -44,6 +44,16 @@ sub search_simple {
return $results;
}

sub search_first {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this called search_first? Can we give it a more descriptive name or add a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that name was copied of search_simple & search_web.... it's basically search_simple but only returns the first element (so web doesn't have to dig through {hits}{hits} structures.

I don't mind changing it, any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

In that case maybe even just search_for_first_result would convey what it's doing a little more clearly?

Copy link
Member

Choose a reason for hiding this comment

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

Might be helpful to add a note explaining when or why we want to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll change the name as suggested.
just to be clear, it's the only usage of search_simple... so technically can be used only as a temp. name until we remove the need for search_simple

This will simplify the reading of /search/web/simple output by
extracting the first element and eliminating the need for WEB
to read an ES output structure.
@mickeyn mickeyn force-pushed the mickey/search_first_endpoint branch from 2642c5d to e6b5c6e Compare July 5, 2017 21:18
@mickeyn mickeyn dismissed oalders’s stale review July 5, 2017 21:19

changed as requested

@oalders oalders merged commit aacc327 into master Jul 10, 2017
@oalders oalders deleted the mickey/search_first_endpoint branch July 10, 2017 22:02
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.

None yet

2 participants