Skip to content

Conversation

@brunoocasali
Copy link
Member

@brunoocasali brunoocasali commented Apr 26, 2022

The general idea is to change tests from this:

  it 'searches with index with searchableAttributes setting' do
    response = index.update_searchable_attributes(['title', 'info.comment'])
    index.wait_for_task(response['uid'])
    response = index.add_documents(documents)
    index.wait_for_task(response['uid'])

    response = index.search('An awesome')

    expect(response['hits'].count).to eq(1)
  end

to this:

  it 'searches with index with searchableAttributes setting' do
    index.update_searchable_attributes(['title', 'info.comment']).then(&method(:wait_for_it))
    index.add_documents(documents).then(&method(:wait_for_it))

    response = index.search('An awesome')

    expect(response['hits'].count).to eq(1)
  end

or this:

  it 'searches with index with searchableAttributes setting' do
    wait_for_it index.update_searchable_attributes(['title', 'info.comment'])
    wait_for_it index.add_documents(documents)

    response = index.search('An awesome')

    expect(response['hits'].count).to eq(1)
  end

@brunoocasali brunoocasali added the maintenance Anything related to maintenance (CI, tests, refactoring...) label Apr 26, 2022
@brunoocasali brunoocasali requested a review from curquiza April 26, 2022 02:37
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Thanks for this addition 🚀

  • Why is this going to be merged to bump-meilisearch-v0.27.0 since it's not related to v0.27.0? Couldn't we merge it to main, and then, merge the commit from merge to bump-meilisearch-v0.27.0? 😊
  • Why not changing the tests in the same PR? 😇

@brunoocasali
Copy link
Member Author

@curquiza

Why not changing the tests in the same PR? 😇

I was thinking to change all the tests after this branch is merged 😈, but I prefer to keep it simple to make it understandable, small PRs lead to better reviews!

Why is this going to be merged to bump-meilisearch-v0.27.0 since it's not related to v0.27.0? Couldn't we merge it to main, and then, merge the commit from merge to bump-meilisearch-v0.27.0? 😊

Not sure why 🤔, I think I did that because of the other branches (I opened all of them on the same day). But I'll apply your suggestion!

@curquiza
Copy link
Member

I was thinking to change all the tests after this branch is merged 😈, but I prefer to keep it simple to make it understandable, small PRs lead to better reviews!

👍

Not sure why 🤔, I think I did that because of the other branches (I opened all of them on the same day). But I'll apply your suggestion!

For the same reason "small PRs lead to better reviews" I think we should put to bump-meilisearch-v0.27.0 only changes related to v0.27.0, otherwise the final review of bump-meilisearch-v0.27.0 will be less easy

@brunoocasali brunoocasali changed the base branch from bump-meilisearch-v0.27.0 to main April 26, 2022 14:38
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

LGTM 🔥🔥

@brunoocasali
Copy link
Member Author

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Apr 26, 2022

@meili-bors meili-bors bot merged commit 5e7924d into main Apr 26, 2022
@meili-bors meili-bors bot deleted the task-helper branch April 26, 2022 16:37
meili-bors bot added a commit that referenced this pull request May 9, 2022
323: Update version for the next release (v0.18.3) r=curquiza a=brunoocasali

This version makes this package compatible with Meilisearch v0.27.0 🎉
Check out the changelog of [Meilisearch v0.27.0](https://github.com/meilisearch/meilisearch/releases/tag/v0.27.0) for more information on the changes.

## 🚀 Enhancements

* Ensure nested field support (#318) `@brunoocasali`
* Add `highlightPreTag`, `highlightPostTag`, `cropMarker`, parameters in the search request (#319) `@brunoocasali`
* Create a helper module to reduce test duplication (#316) `@brunoocasali`

## 🐛 Bug Fixes

* Improve error handling, rescue EPIPE and Net::* errors (#307) `@brunoocasali`

Thanks again to `@brunoocasali!` 🎉


Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Anything related to maintenance (CI, tests, refactoring...)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants