Implement new await synchronous interface #498

ellnix · 2023-10-09T11:04:13Z

Pull Request

Related issue

Fixes #288

While refactoring, inadvertently fixes:

Fixes A test asserts incorrect behavior #499
Fixes Redundant tests in settings_spec #500

PR checklist

Please check if your PR fulfills the following requirements:

Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
Have you read the contributing guidelines?
Have you made sure that the title is accurate and descriptive of the changes?

Changes Checklist

Add soft deprecation messages
Remove bang methods from specs
Add Task "model"
Ensure all tests pass with edits where necessary
Add tests to make sure that all bang methods actually provide deprecation warnings
Return Task model on all methods that return tasks
- Return Task model from all settings setters and getters
Update the documentation with new syntax (if applicable)

codecov · 2023-10-09T15:06:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (dcc99be) to head (75017dc).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #498      +/-   ##
==========================================
+ Coverage   99.81%   99.85%   +0.03%     
==========================================
  Files           9       10       +1     
  Lines         542      671     +129     
==========================================
+ Hits          541      670     +129     
  Misses          1        1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ellnix · 2023-10-12T15:46:11Z

@brunoocasali this is pretty much complete, please feel free to review when you have (a lot of) time. 👨‍💻 🤯

ellnix · 2023-10-16T10:56:32Z

Rebased and force pushed.

brunoocasali

Thanks a lot for your hard work here @ellnix amazing!

I am asking for some changes but it is nothing to be worried about!

lib/meilisearch/client.rb

lib/meilisearch/index.rb

lib/meilisearch/models/task.rb

spec/meilisearch/client/dumps_spec.rb

spec/meilisearch/index/search/nested_fields_spec.rb

spec/meilisearch/models/task_spec.rb

brunoocasali · 2023-11-03T18:46:00Z

spec/meilisearch/models/task_spec.rb

+# frozen_string_literal: true
+
+describe MeiliSearch::Model::Task do
+  let(:new_index_uid) { random_uid }


Can you apply the xUnit structure here? And the https://betterspecs.org guides about the naming of the contexts using with/when?!a

The contexts have been renamed.

About the xUnit structure, I am assuming you specifically mean the setup, exercise, verify, and teardown steps?

setup is done in rspec lets which by design are shared so do not directly appear in a test's body, should I clarify them somehow or have a line in each test explicitly create the correct context for clarity?

exercise and verify are often in the same line of code for brevity, should I expand them?

teardown is also handled by rspec lets, and as far as I know there is no context leaking between specs

brunoocasali

Amazing work @ellnix I think we could make some small modifications, but I was impressed, very good work!

ellnix · 2023-11-29T12:07:33Z

@brunoocasali sorry for taking so long to get back to this. I addressed your feedback and rebased it to main (for the changes since you last reviewed it, start at commit b141ed4).

P. S.: The tests pass but codecov is broken again.

brunoocasali · 2023-12-11T20:50:33Z

let's see if the code coverage will still be broken now!

Also, can you squash the commits @ellnix ?

ellnix · 2023-12-11T22:24:25Z

Also, can you squash the commits @ellnix ?

Squash them into a single commit or just fewer commits? How many commits would you consider ideal? The history might be a little bit hard to follow if only a few commits had +1800 -1200 changes.

lib/meilisearch/utils.rb

spec/meilisearch/models/task_spec.rb

lib/meilisearch/index.rb

lib/meilisearch/models/task.rb

spec/meilisearch/models/task_spec.rb

ellnix · 2023-11-29T11:38:49Z

spec/meilisearch/models/task_spec.rb

+# frozen_string_literal: true
+
+describe MeiliSearch::Model::Task do
+  let(:new_index_uid) { random_uid }


The contexts have been renamed.

About the xUnit structure, I am assuming you specifically mean the setup, exercise, verify, and teardown steps?

setup is done in rspec lets which by design are shared so do not directly appear in a test's body, should I clarify them somehow or have a line in each test explicitly create the correct context for clarity?

exercise and verify are often in the same line of code for brevity, should I expand them?

teardown is also handled by rspec lets, and as far as I know there is no context leaking between specs

brunoocasali · 2023-12-14T02:53:07Z

Squash them into a single commit or just fewer commits? How many commits would you consider ideal? The history might be a little bit hard to follow if only a few commits had +1800 -1200 changes.

A few commits would be great. I don't have a number for that, it is more a feeling than anything. If you squash them based on the group like, you did a lot of the same but in multiple files, so in my mind you would have a single commit of that "theme".

ellnix · 2024-01-10T10:30:01Z

@brunoocasali should be ready for merge, unless you want me to squash it down further.

brunoocasali · 2024-02-14T16:48:48Z

spec/meilisearch/models/task_spec.rb

+        allow(subject).to receive(:refresh)
+        subject.enqueued?
+        expect(subject).not_to have_received(:refresh)


When I mention the xUnit I mean this:

Suggested change

allow(subject).to receive(:refresh)

subject.enqueued?

expect(subject).not_to have_received(:refresh)

allow(subject).to receive(:refresh)

subject.enqueued?

expect(subject).not_to have_received(:refresh)

brunoocasali

Looks good to me, let get this merged @ellnix

Thanks a lot for all the hard work in this repo @ellnix ❤️

ellnix · 2024-02-16T17:28:23Z

I'm not sure this will work, but addressing the conflict with a commit might dismiss the approval review.

bors merge

meili-bors · 2024-02-16T17:28:40Z

Merge conflict.

ellnix · 2024-02-16T17:51:45Z

Give me a second with this, apparently the git pull on my fork didn't go through properly and now everything is messed up.

Refactor all doc methods & specs with Model::Task Update global settings methods to use Model::Task Update ranking_rules methods to use Model::Task Update distinct attribute methods with Model::Task Update searchable attr methods to use Model::Task Update displayed attrs methods with Model::Task Update synonoms methods to use Model::Task Update stop words methods to use Model::Task Update filterable attrs methods to use Model::Task Update sortable attrs methods to use Model::Task

Update typo tolerance methods with Model::Task Update faceting settings methods with Model::Task Update user dict methods to use Model::Task Update (non) separator methods to use Model::Task Update Index#update and #delete to use Model::Task

Replace calls to wait_for_task in specs which I had missed somehow Remove now-redundant TaskHelpers Update some Client methods&specs with Model::Task Add Model::Task to Client#dump and other specs

ellnix · 2024-02-16T18:08:10Z

Well I fixed the git history and force pushed. Sorry about the issues but I don't think there was a way for me to resolve the main conflicts without automatically dismissing the approval review (even when I tried a merge rather than a rebase).

I looked it up, it seems to be a setting (isaacs/github#783 (comment)) but it does not look like it would be a good idea, since it would allow someone with approval to push arbitrary code and then choose to merge.

brunoocasali

LGTM!

ellnix · 2024-02-27T19:40:52Z

bors merge

meili-bors · 2024-02-27T19:42:12Z

Build succeeded:

ellnix changed the title ~~New await sync interface~~ Implement new await synchronous interface Oct 9, 2023

ellnix force-pushed the new-await-sync-interface branch 3 times, most recently from ff3856a to 5fbcab1 Compare October 12, 2023 12:00

ellnix marked this pull request as ready for review October 12, 2023 15:46

ellnix force-pushed the new-await-sync-interface branch from b82e2d1 to f4d63c8 Compare October 16, 2023 10:55

brunoocasali reviewed Nov 3, 2023

View reviewed changes

brunoocasali requested changes Nov 3, 2023

View reviewed changes

ellnix force-pushed the new-await-sync-interface branch from 5e375ff to 5ea3aab Compare November 29, 2023 12:05

ellnix requested a review from brunoocasali November 29, 2023 12:09

ellnix commented Dec 12, 2023

View reviewed changes

ellnix force-pushed the new-await-sync-interface branch from 1db64a5 to 5151609 Compare January 10, 2024 10:28

brunoocasali reviewed Feb 14, 2024

View reviewed changes

brunoocasali previously approved these changes Feb 14, 2024

View reviewed changes

brunoocasali added the enhancement New feature or request label Feb 14, 2024

ellnix dismissed brunoocasali’s stale review via f50f5da February 16, 2024 17:45

ellnix added 3 commits February 16, 2024 18:57

Soft deprecate bang methods with warning

757e7cd

Update specs to use new await syntax

19fa8f4

Write Model::Task and its tests

78f7311

ellnix added 7 commits February 16, 2024 19:01

Remove redundant tests

b1bc1a0

Update more Index methods with Model::Task

4d7c1a3

Update typo tolerance methods with Model::Task Update faceting settings methods with Model::Task Update user dict methods to use Model::Task Update (non) separator methods to use Model::Task Update Index#update and #delete to use Model::Task

Refactor specs with Task#await syntax

45bb7ce

Replace calls to wait_for_task in specs which I had missed somehow Remove now-redundant TaskHelpers Update some Client methods&specs with Model::Task Add Model::Task to Client#dump and other specs

Fix bad deprecation advice on 2 methods

c5a8e92

Fix and rename Models namespace

1b03f73

Clean up and refactor Model::Task spec

75017dc

ellnix force-pushed the new-await-sync-interface branch from 6059ff7 to 75017dc Compare February 16, 2024 18:02

ellnix requested a review from brunoocasali February 16, 2024 18:08

brunoocasali approved these changes Feb 27, 2024

View reviewed changes

meili-bors bot merged commit 65a34c9 into meilisearch:main Feb 27, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new await synchronous interface #498

Implement new await synchronous interface #498

ellnix commented Oct 9, 2023 •

edited

codecov bot commented Oct 9, 2023 •

edited

ellnix commented Oct 12, 2023 •

edited

ellnix commented Oct 16, 2023

brunoocasali left a comment

brunoocasali Nov 3, 2023

ellnix Nov 29, 2023

brunoocasali left a comment

ellnix commented Nov 29, 2023 •

edited

brunoocasali commented Dec 11, 2023

ellnix commented Dec 11, 2023

ellnix Nov 29, 2023

brunoocasali commented Dec 14, 2023

ellnix commented Jan 10, 2024

brunoocasali Feb 14, 2024

brunoocasali left a comment

ellnix commented Feb 16, 2024

meili-bors bot commented Feb 16, 2024

ellnix commented Feb 16, 2024

ellnix commented Feb 16, 2024

brunoocasali left a comment

ellnix commented Feb 27, 2024

meili-bors bot commented Feb 27, 2024

Implement new await synchronous interface #498

Implement new await synchronous interface #498

Conversation

ellnix commented Oct 9, 2023 • edited

Pull Request

Related issue

PR checklist

Changes Checklist

codecov bot commented Oct 9, 2023 • edited

Codecov Report

ellnix commented Oct 12, 2023 • edited

ellnix commented Oct 16, 2023

brunoocasali left a comment

Choose a reason for hiding this comment

brunoocasali Nov 3, 2023

Choose a reason for hiding this comment

ellnix Nov 29, 2023

Choose a reason for hiding this comment

brunoocasali left a comment

Choose a reason for hiding this comment

ellnix commented Nov 29, 2023 • edited

brunoocasali commented Dec 11, 2023

ellnix commented Dec 11, 2023

ellnix Nov 29, 2023

Choose a reason for hiding this comment

brunoocasali commented Dec 14, 2023

ellnix commented Jan 10, 2024

brunoocasali Feb 14, 2024

Choose a reason for hiding this comment

brunoocasali left a comment

Choose a reason for hiding this comment

ellnix commented Feb 16, 2024

meili-bors bot commented Feb 16, 2024

ellnix commented Feb 16, 2024

ellnix commented Feb 16, 2024

brunoocasali left a comment

Choose a reason for hiding this comment

ellnix commented Feb 27, 2024

meili-bors bot commented Feb 27, 2024

ellnix commented Oct 9, 2023 •

edited

codecov bot commented Oct 9, 2023 •

edited

ellnix commented Oct 12, 2023 •

edited

ellnix commented Nov 29, 2023 •

edited