-
Notifications
You must be signed in to change notification settings - Fork 107
feat: add missing params in SearchQuery #817
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds two nullable properties to the SearchQuery contract: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #817 +/- ##
==========================================
- Coverage 89.78% 87.63% -2.16%
==========================================
Files 59 63 +4
Lines 1449 1569 +120
==========================================
+ Hits 1301 1375 +74
- Misses 148 194 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Contracts/SearchQuery.php (1)
439-467: Minor: Consider adding@returnPHPDoc for consistency.The getter and setter implementations are correct and follow the established pattern of returning
selffor method chaining. However, most other setters in this class include a@return $thisPHPDoc comment (e.g., lines 123, 134, 156), which is missing here.Consider applying this diff for consistency:
+ /** + * @return $this + */ public function setRetrieveVectors(?bool $retrieveVectors): self { $this->retrieveVectors = $retrieveVectors; return $this; } /** * @return array<string, mixed>|null */ public function getMedia(): ?array { return $this->media; } /** * @param array<string, mixed>|null $media + * + * @return $this */ public function setMedia(?array $media): self { $this->media = $media; return $this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Contracts/SearchQuery.php(4 hunks)tests/Contracts/SearchQueryTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Contracts/SearchQuery.php (2)
src/Contracts/DocumentsQuery.php (1)
setRetrieveVectors(96-101)src/Contracts/SimilarDocumentsQuery.php (1)
setRetrieveVectors(137-142)
tests/Contracts/SearchQueryTest.php (1)
src/Contracts/SearchQuery.php (4)
SearchQuery(7-536)setRetrieveVectors(444-449)toArray(502-535)setMedia(462-467)
🔇 Additional comments (4)
tests/Contracts/SearchQueryTest.php (2)
240-249: LGTM! Test follows established patterns.The test correctly verifies both boolean values are properly serialized to the array output, consistent with similar boolean property tests in this file.
251-257: LGTM! Test correctly verifies media array serialization.The test follows the established pattern for array properties and uses a realistic media structure with MIME type and data URI.
src/Contracts/SearchQuery.php (2)
114-119: LGTM! Property declarations are well-typed.Both properties follow the established pattern with appropriate nullable types and PHPDoc annotations. The flexible
array<string, mixed>type for media accommodates various multimedia configuration structures.
497-499: LGTM! PHPDoc accurately reflects new return type fields.The documentation correctly adds the optional
retrieveVectorsandmediafields to the return type annotation, with types matching the property declarations.
267a69e to
f0f5339
Compare
|
Fixes #816 right? |
| * | ||
| * More info: https://www.meilisearch.com/docs/reference/api/experimental-features | ||
| * | ||
| * @param array<string, mixed>|null $media |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to me mixed looks to broad. also i failed to find any info about this media
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, same 🙁 Maybe @Strift do you have more information on how the media's param is supposed to be structured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can take a look at how I implemented it in the JS SDK, if that can be helpful: https://github.com/meilisearch/meilisearch-js/pull/1998/files
See src/types/types.ts for the types and tests/multi_modal_search.test.ts for the examples.
Correct! |
Add missing params
retrieveVectorsandmediainSearchQuery.Summary by CodeRabbit
New Features
Tests