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

Properly set limit in Kaminari #395

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

ellnix
Copy link
Collaborator

@ellnix ellnix commented Feb 10, 2025

Fixes regression in #376
Fixes #394

@ellnix ellnix requested a review from brunoocasali February 10, 2025 19:50
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (385b278) to head (0e06892).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
+ Coverage   89.19%   89.20%   +0.01%     
==========================================
  Files          13       13              
  Lines         768      769       +1     
==========================================
+ Hits          685      686       +1     
  Misses         83       83              

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

offset = 0 if offset.nil?
limit = 0 if options[:per_page].nil?
array = new results, limit: limit, offset: offset, total_count: total_hits
unless MeiliSearch::Rails.active?

Choose a reason for hiding this comment

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

Suggested change
unless MeiliSearch::Rails.active?
unless Meilisearch::Rails.active?

To maintain compatibility with #384

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would if I knew which one would be merged first... Besides, there could be a patch level release with this PR while the rename is done in 0.15, that way 0.14 can have a stabler final version.

@wesharper
Copy link

wesharper commented Feb 13, 2025

Out of pure curiosity, why is the limit being set by default to 1? Is there a configuration value that can fetch some environment-level default set by either Meilisearch or Kaminari?

@ellnix
Copy link
Collaborator Author

ellnix commented Feb 13, 2025

@wesharper The specific value of the limit doesn't matter, it's really an extension of the NullObject pattern that this gem uses when you disable Meilisearch, I used 1 as a simple value that would not cause an error.

The problem this solves is the NullObject returns itself however you interact with it, and those instances end up in Kaminari causing all kinds of trouble. See #375

Edit: 0 works just as well, I just kept it consistent with will_paginate.

@wesharper
Copy link

Thanks for the context, @ellnix.

@ellnix
Copy link
Collaborator Author

ellnix commented Mar 7, 2025

bors merge

@meili-bors meili-bors bot merged commit 6afcd40 into meilisearch:main Mar 7, 2025
12 checks passed
@ellnix ellnix added the bug Something isn't working label Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

current_page Method Missing After Upgrading meilisearch-rails
3 participants