Skip to content
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

Rename MeiliSearch to Meilisearch #384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ellnix
Copy link
Collaborator

@ellnix ellnix commented Jan 14, 2025

Depends on meilisearch-ruby 0.30 where MeiliSearch was also renamed. Used this shell command to automatically rename, and then used the git diff to manually look through and make sure nothing was wrong:

find lib/ -type f -exec sed -i 's/MeiliSearch/Meilisearch/g' {} \; 

(of course also ran it for spec/ and playground/ after making sure the soft deprecation allowed all tests to pass).

Ran into issues with the regular const_missing + const_get deprecation having issues with autoload-ed methods. Added a temporary workaround just for the Rails module. With this workaround there may be cases where users are not warned for using MeiliSearch, which should be investigated.

Pull Request

Related issue

Fixes #347

@ellnix ellnix added the breaking-change The related changes are breaking for the users label Jan 14, 2025
@ellnix
Copy link
Collaborator Author

ellnix commented Jan 14, 2025

Please do not merge until the time comes to update to meilisearch-ruby 0.30. Development can happen in the meantime on the current version and I'll make sure to keep this PR updated.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 93.75000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.56%. Comparing base (5450df2) to head (6df283f).

Files with missing lines Patch % Lines
lib/meilisearch-rails.rb 94.73% 1 Missing ⚠️
lib/meilisearch/rails/pagination/kaminari.rb 66.66% 1 Missing ⚠️
lib/meilisearch/rails/pagination/will_paginate.rb 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
+ Coverage   89.54%   89.56%   +0.01%     
==========================================
  Files          13       13              
  Lines         775      776       +1     
==========================================
+ Hits          694      695       +1     
  Misses         81       81              

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ellnix ellnix force-pushed the rename-top-level-module branch from 0b48285 to b09944e Compare February 5, 2025 19:09
@ellnix ellnix marked this pull request as ready for review February 5, 2025 19:16
@ellnix ellnix force-pushed the rename-top-level-module branch from b09944e to 44542b6 Compare February 5, 2025 19:16
@ellnix
Copy link
Collaborator Author

ellnix commented Feb 5, 2025

I don't think we can put this off much longer, all the tests pass in the current state, I don't think this breaks anything ( 👀 )

I'll start work on federated search on top of this.

@ellnix ellnix mentioned this pull request Feb 6, 2025
4 tasks
@wesharper
Copy link

@ellnix, thanks for all your work on this and #393!

@brunoocasali @curquiza, I'm very sorry to ping you directly and would be happy to contribute my and my team's resources in any way necessary to help get this and #393 over the line. I know that you must be spread thin and I can't sympathize enough with the burden of maintaining open source projects, so I really don't intend to be a pest.

My team relies on federated search as a hard requirement, and would gain some immediate tangible benefits from switching to the rails gem and refactoring our codebase. We've already done some initial cleanup in a demo branch and the team is really happy with the results.

@ellnix
Copy link
Collaborator Author

ellnix commented Feb 13, 2025

My team relies on federated search as a hard requirement, and would gain some immediate tangible benefits from switching to the rails gem and refactoring our codebase. We've already done some initial cleanup in a demo branch and the team is really happy with the results.

@wesharper Do you mean that you are not currently using the rails gem at all and just using ms-ruby? Or that you have your own federated search implementation on top of ms-rails?

@wesharper
Copy link

@ellnix, we've built our own version of federated search on top of ms-ruby before it was implemented there (thanks for that, btw). But we've also built our own model sync and fetching logic to play nicely with ActiveRecord, which we'd like to remove all at once by switching wholesale to the rails gem (vs the ruby gem).

@ellnix
Copy link
Collaborator Author

ellnix commented Feb 13, 2025

@wesharper Wow, it must be a lot of hard to work to manually keep activerecord in sync with meilisearch. If you need help with the transition or maintenance of your current solution, and you are open source feel free to reach out. I've been curious to see examples of how teams keep ms and ar in sync in real production code, there are so many edge cases.

@wesharper
Copy link

@ellnix thanks so much for the offer. Unfortunately we're closed source, but happy to chat more offline.

Luckily, it hasn't been too cumbersome, although we did have our search feature catastrophically fail once or twice early on. Luckily, we are able to get away with cutting some corners for various reasons, but would like to close some of the most glaring and obvious gaps now that we are going to be able to switch to this gem.

@ellnix
Copy link
Collaborator Author

ellnix commented Feb 13, 2025

but happy to chat more offline.

(at)ell.nix on discord (you can also find me on the meilisearch discord's members list)

The email on my commits also works.

brunoocasali
brunoocasali previously approved these changes Mar 4, 2025
Depends on meilisearch-ruby 0.30 where MeiliSearch was also renamed.
Used this shell command to automatically rename, and then used the git
diff to manually look through and make sure nothing was wrong:
find lib/ -type f -exec sed -i 's/MeiliSearch/Meilisearch/g' {} \;
(of course also ran it for spec/ and playground/ after making sure the
soft deprecation allowed all tests to pass).

Ran into issues with the regular `const_missing` + `const_get`
deprecation having issues with autoload-ed methods. Added a temporary
workaround just for the Rails module. With this workaround there may be
cases where users are not warned for using MeiliSearch, which should be
investigated.
@ellnix
Copy link
Collaborator Author

ellnix commented Mar 14, 2025

Rebased and resolved conflicts.

@ellnix ellnix requested a review from brunoocasali March 14, 2025 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename root module to Meilisearch instead of MeiliSearch
3 participants