Refactor Meilisearch::Index class into focused modules#677
Refactor Meilisearch::Index class into focused modules#677psylone wants to merge 6 commits intomeilisearch:mainfrom
Meilisearch::Index class into focused modules#677Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #677 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 16 +6
Lines 809 835 +26
=========================================
+ Hits 809 835 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| end | ||
| end | ||
|
|
||
| include Compact |
There was a problem hiding this comment.
Hi there! I loved your PR altough I have one question regarding the class reopening during these module definitions. I think I have never seen this being used like this. Is there any benefit to it over regular mixin usage?
There was a problem hiding this comment.
Thanks for your feedback! Let me explain the reasoning behind this decision.
First of all, we need to ensure that Meilisearch::Index inherits from Meilisearch::HTTPRequest, which means lib/meilisearch/index.rb must be loaded before the submodules since they reopen class Index.
That's why we can't place require 'meilisearch/index/compact' at the top of lib/meilisearch/index.rb: the modules must be required after the class Index < HTTPRequest definition.
We could place include Compact right after the require statements, but I prefer to put it inside the Compact module definition to make it self-contained and keep Meilisearch::Index cleaner.
You might also wonder: why not use plain mixins like Meilisearch::IndexCompact and require/include them the usual way? That's certainly possible, and I considered it.
However, I find it cleaner to define Meilisearch::Index::Compact as a nested module and place it under the lib/meilisearch/index folder - it better reflects the logical hierarchy.
| Max: 24 | ||
|
|
||
| # Offense count: 4 | ||
| # Offense count: 3 |
Pull Request
Related issue
Fixes #676
What does this PR do?
Meilisearch::IndexclassPR checklist
Please check if your PR fulfills the following requirements: