Added support for search option 'request_cache' #1243

Merged
merged 1 commit into from Mar 14, 2017

Conversation

Projects
None yet
5 participants
@ilanrivers
Contributor

ilanrivers commented Feb 1, 2017

No description provided.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Feb 2, 2017

Owner

@ilanrivers Thanks for the contribution. Could you also add a quick integration test for this one?

Owner

ruflin commented Feb 2, 2017

@ilanrivers Thanks for the contribution. Could you also add a quick integration test for this one?

@ilanrivers

This comment has been minimized.

Show comment
Hide comment
@ilanrivers

ilanrivers Feb 2, 2017

Contributor

@ruflin Where exactly would this best fit? I do not see any other integration test for the other options.

Contributor

ilanrivers commented Feb 2, 2017

@ruflin Where exactly would this best fit? I do not see any other integration test for the other options.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Feb 2, 2017

Owner

I think it should be in https://github.com/ruflin/Elastica/blob/master/test/lib/Elastica/Test/SearchTest.php There seem to be at least some tests which cover other constants.

Owner

ruflin commented Feb 2, 2017

I think it should be in https://github.com/ruflin/Elastica/blob/master/test/lib/Elastica/Test/SearchTest.php There seem to be at least some tests which cover other constants.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 13, 2017

Collaborator

I don't think this needs a test because it's difficult to test if ES used a cache or not internally which this flag controls. So this is good to merge IMO.

Collaborator

Tobion commented Mar 13, 2017

I don't think this needs a test because it's difficult to test if ES used a cache or not internally which this flag controls. So this is good to merge IMO.

@Zyqsempai

This comment has been minimized.

Show comment
Hide comment
@Zyqsempai

Zyqsempai Mar 14, 2017

Contributor

Guys we need to merge it, it is a really long awaited feature.
Tests are not needed here, they can be only formal, "Request passed or not".

Contributor

Zyqsempai commented Mar 14, 2017

Guys we need to merge it, it is a really long awaited feature.
Tests are not needed here, they can be only formal, "Request passed or not".

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 14, 2017

Owner

I'm ok with merging it. But we need a CHANGELOG entry. Either @ilanrivers has time to do it or @Zyqsempai or @Tobion perhaps have the time to do a follow up PR with the CHANGELOG? Or I can do it next week ... @Tobion Feel free to merge it.

Owner

ruflin commented Mar 14, 2017

I'm ok with merging it. But we need a CHANGELOG entry. Either @ilanrivers has time to do it or @Zyqsempai or @Tobion perhaps have the time to do a follow up PR with the CHANGELOG? Or I can do it next week ... @Tobion Feel free to merge it.

@Tobion Tobion merged commit c7939d1 into ruflin:master Mar 14, 2017

3 checks passed

codecov/patch 100% of diff hit (target 83.91%)
Details
codecov/project 83.91% (+<.01%) compared to 96ab8aa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Tobion added a commit that referenced this pull request Mar 14, 2017

Zyqsempai added a commit to Zyqsempai/Elastica that referenced this pull request Mar 14, 2017

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 14, 2017

Collaborator

Added the changelog

Collaborator

Tobion commented Mar 14, 2017

Added the changelog

mhernik pushed a commit to mhernik/Elastica that referenced this pull request Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment