Skip to content

Start testing & supporting MongoDB #410

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 14, 2025
Merged

Start testing & supporting MongoDB #410

merged 7 commits into from
Apr 14, 2025

Conversation

ellnix
Copy link
Collaborator

@ellnix ellnix commented Mar 18, 2025

Pull Request

Related issue

Begins to fix #175

@ellnix ellnix added the enhancement New feature or request label Mar 18, 2025
@codecov Codecov
Copy link

codecov bot commented Mar 18, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.69%. Comparing base (a35652c) to head (5a78e6e).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
+ Coverage   90.43%   90.69%   +0.25%     
==========================================
  Files          14       14              
  Lines         847      849       +2     
==========================================
+ Hits          766      770       +4     
+ Misses         81       79       -2

☔ 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.

brunoocasali
brunoocasali previously approved these changes Mar 24, 2025
View reviewed changes
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

I would love to have a way to not have more 1 billion of if/elses in theses files 😅. Let's make it first and ensure everything is very well tested, so we can improve the gem once and for all 🫡

@ellnix
Copy link
Collaborator Author

ellnix commented Mar 24, 2025

I would love to have a way to not have more 1 billion of if/elses in theses files 😅. Let's make it first and ensure everything is very well tested, so we can improve the gem once and for all 🫡

Absolutely! On principle I'm starting all changes from the tests first, afterward I'm planning to use composition for storage backends, i. e. when meilisearch is called we have an if statement that decides which class of storage backend is loaded for this particular model. Unless I'm mistaken this is an excellent situation for composition.

Also I can remove at least one of the if statements now that I know what was going on with after_destroy_commit in mongoid (mongodb/mongoid#5968).

@ellnix
Use after_commit instead of after_destroy_commit
5a78e6e 
The bugfix on Mongoid is not released yet, but that should not matter
since there is no real reason to turn this back to after_destroy_commit.

The behavior is equivalent.
@ellnix
Copy link
Collaborator Author

ellnix commented Mar 28, 2025 •
edited
Loading

Codecov is happy, and so is rubocop. The after_commit callback is fixed.

I'll go ahead with the rest of the work on Mongoid on another PR if you have no notes on this one as a foundation @brunoocasali

brunoocasali
brunoocasali approved these changes Apr 1, 2025
View reviewed changes
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

LGTM

@ellnix ellnix added the skip-changelog The PR will not appear in the release changelogs label Apr 2, 2025
@ellnix
Copy link
Collaborator Author

ellnix commented Apr 2, 2025

bors merge

1 similar comment
@ellnix
Copy link
Collaborator Author

ellnix commented Apr 2, 2025

bors merge

@ellnix
Copy link
Collaborator Author

ellnix commented Apr 3, 2025

@curquiza can you take a look and see what's happening with Bors?

@ellnix ellnix mentioned this pull request Apr 4, 2025
Draft
@curquiza curquiza merged commit 1483ccf into meilisearch:main Apr 14, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunoocasali brunoocasali brunoocasali approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request skip-changelog The PR will not appear in the release changelogs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure mongodb is correctly supported
3 participants
@ellnix @brunoocasali @curquiza