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

Add decorator functionality #27

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ AllCops:
NewCops: enable

Metrics/AbcSize:
Max: 25
Max: 30

# DSLs inheritly uses long blocks, like describe and contexts in RSpec,
# therefore disable this rule for specs and for the gemspec file
Expand All @@ -13,22 +13,22 @@ Metrics/BlockLength:
- spec/**/*

Metrics/ClassLength:
Max: 200
Max: 205
Copy link
Member

Choose a reason for hiding this comment

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

It's probably really time for us to start splitting this super-class into smaller ones.. but that's not the job of this PR, so I think it's fine to adjust the rule for now and we'll reset it back when working on #1 where we also want to split the class up :)


Metrics/CyclomaticComplexity:
Max: 15

Metrics/MethodLength:
Max: 25
Max: 30

Metrics/ParameterLists:
Max: 7
Max: 8

Metrics/PerceivedComplexity:
Max: 13
Max: 14

Layout/LineLength:
Max: 80
Max: 95
Copy link
Member

Choose a reason for hiding this comment

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

This is a rule I would not want to adjust as it makes usage of some tools (e.g. diff tools) harder. I believe those lines that now got longer than these 80 characters should be easy enough to break (e.g. here we could add add the record_decorator into a new line).


Naming/VariableNumber:
EnforcedStyle: snake_case
Expand Down
26 changes: 18 additions & 8 deletions lib/rails_cursor_pagination/paginator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,19 @@ class InvalidCursorError < ParameterError; end
# ```
# @param order [Symbol, nil]
# Ordering to apply, either `:asc` or `:desc`. Defaults to `:asc`.
# @param record_decorator [Object, proc { |obj| obj.itself }]
# Object to which each paginated record will be passed to. Default behavior
# simply returns the record itself. Object must be callable.
Comment on lines +45 to +47
Copy link
Member

Choose a reason for hiding this comment

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

While technically any object that responds to the call method is supported here, I would suggest maybe documenting it as accepting a Proc as it will probably be the most used form and might be easier to understand than any callable object (with that logic, anything could be Object as long as it responds to the right methods 😉 ). Optionally we can also add #call to say that any other object that responds to #call is being accepted.

Suggested change
# @param record_decorator [Object, proc { |obj| obj.itself }]
# Object to which each paginated record will be passed to. Default behavior
# simply returns the record itself. Object must be callable.
# @param record_decorator [Proc(ActiveRecord::Base), #call]
# This can be used to change the what is being returned as the page data.
# If provided, this proc will be invoked for each record with the record
# as its only parameter, and the result of it will be returned. Defaults
# to just return the record itself.

What I sometimes find useful is to use this YARD parser to see if the annotation is in a valid format 😉 Sadly, I could not find a super elegant way to document a Proc with its accepted arguments and return value, but I think the proposed solution comes as close as possible to what we want to express.

#
# @raise [RailsCursorPagination::Paginator::ParameterError]
# If any parameter is not valid
def initialize(relation, first: nil, after: nil, last: nil, before: nil,
order_by: nil, order: nil)
order_by: nil, order: nil, record_decorator: :itself.to_proc)
order_by ||= :id
order ||= :asc

ensure_valid_params!(relation, first, after, last, before, order)
ensure_valid_params!(relation, first, after, last, before, order,
record_decorator)

@order_field = order_by
@order_direction = order
Expand All @@ -64,6 +68,8 @@ def initialize(relation, first: nil, after: nil, last: nil, before: nil,
last ||
RailsCursorPagination::Configuration.instance.default_page_size

@record_decorator = record_decorator

@memos = {}
end

Expand Down Expand Up @@ -99,10 +105,12 @@ def fetch(with_total: false)
# Optional, cannot be combined with `after`
# @param order [Symbol]
# Optional, must be :asc or :desc
# @param record_decorator [Object, proc { |obj| obj.itself }]
# Optional, must respond to `.call`

Choose a reason for hiding this comment

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

Passing a record_decorator is optional in #initialize, but #ensure_valid_params! actually requires it.

#
# @raise [RailsCursorPagination::Paginator::ParameterError]
# If any parameter is not valid
def ensure_valid_params!(relation, first, after, last, before, order)
def ensure_valid_params!(relation, first, after, last, before, order, record_decorator)
unless relation.is_a?(ActiveRecord::Relation)
raise ParameterError,
'The first argument must be an ActiveRecord::Relation, but was '\
Expand All @@ -127,6 +135,9 @@ def ensure_valid_params!(relation, first, after, last, before, order)
if last.present? && last.negative?
raise ParameterError, "`last` cannot be negative, but was `#{last}`"
end
unless record_decorator.respond_to?(:call)
raise ParameterError, '`record_decorator` must respond to .call'
end

true
end
Expand All @@ -143,15 +154,16 @@ def page_info
}
end

# Get the records for the given page along with their cursors
# Get the records for the given page along with their cursors.
# Returned data will have been passed to record_decorator.
#
# @return [Array<Hash>] List of hashes, each with a `cursor` and `data`
def page
memoize :page do
records.map do |item|
{
cursor: cursor_for_record(item),
data: item
data: @record_decorator.call(item)
}
end
end
Expand Down Expand Up @@ -393,9 +405,7 @@ def relation_with_cursor_fields

relation = @relation

unless @relation.select_values.include?(:id)
relation = relation.select(:id)
end
relation = relation.select(:id) unless @relation.select_values.include?(:id)

if custom_order_field? && !@relation.select_values.include?(@order_field)
relation = relation.select(@order_field)
Expand Down
29 changes: 29 additions & 0 deletions spec/rails_cursor_pagination/paginator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,13 @@
include_examples 'for a ParameterError with the right message',
'`last` cannot be negative, but was `-4`'
end

context 'passing an invalid `record_decorator`' do
let(:params) { super().merge(record_decorator: 'no') }

include_examples 'for a ParameterError with the right message',
'`record_decorator` must respond to .call'
end
end
end

Expand Down Expand Up @@ -274,6 +281,28 @@
expect(subject[:total]).to eq expected_total
end
end

context 'when passing a valid proc as `&record_decorator`' do
let(:params) { { record_decorator: :class.to_proc } }

it 'returns decorated records' do
expect(subject[:page].pluck(:data)).to all be Post
end
end

context 'when passing a valid lambda as `&record_decorator`' do
let(:id_incrementer) { ->(item) { { next_id: item.id + 1 } } }
let(:params) { super().merge(record_decorator: id_incrementer) }
let(:expected_decorated_response) do
expected_posts.map do |post|
id_incrementer.call(post)
end
Comment on lines +297 to +299

Choose a reason for hiding this comment

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

💅

Suggested change
expected_posts.map do |post|
id_incrementer.call(post)
end
expected_posts.map(&id_incrementer)

end

it 'returns decorated records' do
expect(subject[:page].pluck(:data)).to eq expected_decorated_response
end
end
end

shared_examples_for 'a well working query that also supports SELECT' do
Expand Down