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

kaminari 1.0.1 page_entries_info(results) ArgumentError: wrong number of arguments (given 1, expected 0) #934

Closed
teohm opened this Issue Jun 14, 2017 · 2 comments

Comments

2 participants
@teohm

teohm commented Jun 14, 2017

The latest kaminari (v1.0.1) has changed Kaminari::ActiveRecordRelationMethods#entry_name to accept a new optional options argument. See: https://github.com/kaminari/kaminari/blob/v1.0.1/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb#L6-L9

And kaminari view helper page_entries_info(results) is now calling Searchkick::Results#entry_name by passing in this options argument. See: https://github.com/kaminari/kaminari/blob/v1.0.1/kaminari-core/lib/kaminari/helpers/helper_methods.rb#L104

But because Searchkick::Results#entry_name does not accept argument, it raises error:
https://github.com/ankane/searchkick/blob/master/lib/searchkick/results.rb#L130-L132

ArgumentError: wrong number of arguments (given 1, expected 0)

Proposed solution:

I propose to modify Searchkick::Results#entry_name to match Kaminari::ActiveRecordRelationMethods#entry_name implementation:

   def entry_name(options = {})
      default = options[:count] == 1 ? model_name.human : model_name.human.pluralize
      model_name.human(options.reverse_merge(default: default))
    end

What do you think?

@ankane ankane closed this in 0422c61 Jun 14, 2017

@ankane

This comment has been minimized.

Show comment
Hide comment
@ankane

ankane Jun 14, 2017

Owner

Hey @teohm, thanks for the detailed report and proposed fix 💯

I've added an extra conditional for backward compatibility, which can probably be removed in Searchkick 3, as well as tests.

Owner

ankane commented Jun 14, 2017

Hey @teohm, thanks for the detailed report and proposed fix 💯

I've added an extra conditional for backward compatibility, which can probably be removed in Searchkick 3, as well as tests.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Jun 15, 2017

@ankane sweet, looks great! 👍

teohm commented Jun 15, 2017

@ankane sweet, looks great! 👍

mikelkew added a commit to mikelkew/searchkick that referenced this issue Jul 18, 2017

Merge commit '9c957ffd9a6bc5c356ead1ea445cac2e9b6d4369'
* commit '9c957ffd9a6bc5c356ead1ea445cac2e9b6d4369': (25 commits)
  Prefer default_fields if specified [skip ci]
  Use Elasticsearch 5.5 as default for tests [skip ci]
  Upgraded ES6 test version [skip ci]
  Clarify error message [skip ci]
  Ensure _all isn't indexed in ES 6 [skip ci]
  Added _all and default_fields options
  Version bump to 2.3.1
  Updated changelog [skip ci]
  Don't call minimum twice, and handle no record case better
  Switch to find_in_batches and find the first PK to see if's a Numeric
  Clarify filterable and add test
  Updated filterable docs [skip ci]
  Correctly pass the id of the batch
  Remove un-necessary search information from the Sku model
  Reindex non-numerical PK's in batches
  Add failing test for non-integer primary key models
  Updated readme for conversions_term [skip ci]
  Add support for conversions boost term (#933)
  Fixed page_view_entries for Kaminari - fixes #934
  Added test for no fields [skip ci]
  ...

mikelkew added a commit to mikelkew/searchkick that referenced this issue Jul 18, 2017

Merge branch 'master' into handle_hashie_prod
* master: (26 commits)
  Prefer default_fields if specified [skip ci]
  Use Elasticsearch 5.5 as default for tests [skip ci]
  Upgraded ES6 test version [skip ci]
  Clarify error message [skip ci]
  Ensure _all isn't indexed in ES 6 [skip ci]
  Added _all and default_fields options
  Version bump to 2.3.1
  Updated changelog [skip ci]
  Don't call minimum twice, and handle no record case better
  Switch to find_in_batches and find the first PK to see if's a Numeric
  Clarify filterable and add test
  Updated filterable docs [skip ci]
  Correctly pass the id of the batch
  Remove un-necessary search information from the Sku model
  Reindex non-numerical PK's in batches
  Add failing test for non-integer primary key models
  Updated readme for conversions_term [skip ci]
  Add support for conversions boost term (#933)
  Fixed page_view_entries for Kaminari - fixes #934
  Added test for no fields [skip ci]
  ...

mikelkew added a commit to mikelkew/searchkick that referenced this issue Jul 18, 2017

Merge branch 'master' into handle_hashie
* master: (26 commits)
  Prefer default_fields if specified [skip ci]
  Use Elasticsearch 5.5 as default for tests [skip ci]
  Upgraded ES6 test version [skip ci]
  Clarify error message [skip ci]
  Ensure _all isn't indexed in ES 6 [skip ci]
  Added _all and default_fields options
  Version bump to 2.3.1
  Updated changelog [skip ci]
  Don't call minimum twice, and handle no record case better
  Switch to find_in_batches and find the first PK to see if's a Numeric
  Clarify filterable and add test
  Updated filterable docs [skip ci]
  Correctly pass the id of the batch
  Remove un-necessary search information from the Sku model
  Reindex non-numerical PK's in batches
  Add failing test for non-integer primary key models
  Updated readme for conversions_term [skip ci]
  Add support for conversions boost term (#933)
  Fixed page_view_entries for Kaminari - fixes #934
  Added test for no fields [skip ci]
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment