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

Allow for explain to be set on a query #119

Closed
wants to merge 3 commits into from

Conversation

kyle-mccarthy
Copy link
Contributor

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues n/a

Allow for explain to be set on a query, so you can see how the score for a certain document is calculated.

https://www.elastic.co/guide/en/elasticsearch/reference/current/search-explain.html

Can be used by ElasticActiveRecord::find()->query(...)->explain(true)->all();.

Copy link
Member

@cebe cebe left a comment

Choose a reason for hiding this comment

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

change looks good except phpdoc annotations. Could you also write a small test that uses this feature?

@@ -193,6 +193,14 @@ public function getHighlight()
}

/**
* @return array|null An explanation for each hit on how its score was computed
*/
Copy link
Member

Choose a reason for hiding this comment

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

need @since 2.0.5 annotation.

/**
* @var Enables explanation for each hit on how its score was computed.
* @see https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-explain.html
*/
Copy link
Member

Choose a reason for hiding this comment

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

need @since 2.0.5 annotation.

* @param $explain
* @return $this
* @see $explain
*/
Copy link
Member

Choose a reason for hiding this comment

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

need @since 2.0.5 annotation.

@cebe cebe added the type:enhancement Enhancement label Jan 7, 2017
@cebe cebe added this to the 2.0.5 milestone Jan 7, 2017
@kyle-mccarthy
Copy link
Contributor Author

@cebe Okay I have updated the annotations and added in two simple test cases to check for the existence of _explanation on the query when it is set and to ensure it doesn't exist when not set. Let me know if you see anything else.

@cebe
Copy link
Member

cebe commented Jan 9, 2017

looks good to me, thanks! If you add a CHANGLOG line I'll merge it right away.

@@ -171,6 +171,12 @@ class Query extends Component implements QueryInterface
* @since 2.0.4
*/
public $options = [];
/**
* @var Enables explanation for each hit on how its score was computed.
Copy link
Member

Choose a reason for hiding this comment

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

oh, missed that. this should include the type: @var bool


/**
* Explain for how the score of each document was computer
* @param $explain
Copy link
Member

Choose a reason for hiding this comment

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

same here: @param bool $explain

@kyle-mccarthy
Copy link
Contributor Author

Done and not a problem!

@cebe cebe closed this in 2b79ecb Jan 9, 2017
@cebe
Copy link
Member

cebe commented Jan 9, 2017

Merged, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants