-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add decorator functionality #27
Conversation
…cords. Record Decorator needs to be something that can be `.call`ed and that expects a record as an argument.
expected_posts.map do |post| | ||
id_incrementer.call(post) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅
expected_posts.map do |post| | |
id_incrementer.call(post) | |
end | |
expected_posts.map(&id_incrementer) |
@@ -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` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @roykillany! As already discussed before, I think this is a super valuable addition to the gem.
We also talked about what the right place to pass this decorator would be, and while it might make more sense to actually pass it to the #fetch
method, it would also require some bigger refactoring regarding the memoization. I suppose that is something that can be done when we tackle #1 during which we'll also further split up this class. That will likely have to be a breaking change.
What I think is still missing from this PR before we can merge it is a section in the README.md in the Usage section detailing this new option and explaining how and why to use it. E.g. something like:
### Decorating the response
By default, the hash returned by the `#fetch` method will just contain the plain `ActiveRecord::Base` model instances that were fetched from the database.
Using a `#select` statement on the query, we can decide what fields should be fetched and thereby somewhat reduce the data returned.
But often times we might want to be able to also expose data that e.g. is calculated by a method or transform a given datum before returning it.
That is where the `record_decorator` can come in handy.
It allows us to pass a proc / lambda or any other callable object which will then be invoked for each record that is retrieved from the database.
Whatever this proc returns will then be returned as `data` on the `page` objects.
For an example, assume that our `Post` model has a method `#urn` that returns a unique string identifying each post based on its type and ID.
And imagine we want to expose this information along with the `author` from the DB but call the field `name`.
To achieve that, we can define a proc that will do just that:
```ruby
decorator = ->(record) { { urn: record.urn, name: record.author } }
RailsCursorPagination::Paginator
.new(posts, record_decorator: decorator)
.fetch
\```
This would then expose return something similar to this response:
```ruby
{
page_info: {
has_previous_page: false,
has_next_page: true,
start_cursor: "MQ==",
end_cursor: "MTA="
},
page: [
{ cursor: "MQ==", data: { urn: "post:1", author: "Sam" } },
{ cursor: "Mg==", data: { urn: "post:2", author: "Jace" } },
...
]
}
\```
And then a quick update to the CHANGELOG.md
-> Unreleased section detailing the change, and from my side, we should be good to merge it :)
|
||
Layout/LineLength: | ||
Max: 80 | ||
Max: 95 |
There was a problem hiding this comment.
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).
@@ -13,22 +13,22 @@ Metrics/BlockLength: | |||
- spec/**/* | |||
|
|||
Metrics/ClassLength: | |||
Max: 200 | |||
Max: 205 |
There was a problem hiding this comment.
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 :)
# @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. |
There was a problem hiding this comment.
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.
# @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.
ill reopen this when i have time to work on it |
Background
The particular use case of presenting/decorating the fetched records is one that is not currently accounted for. I thought it would make good sense to be able to supply some decorator that the
Paginator
could use to change the returned data.Changes
decorator
kwargdecorator
on eachitem
(https://github.com/xing/rails_cursor_pagination/pull/27/files#diff-c201f4d60dd83e98535d880e8d9b9ad479792383bb84e0cddd4a9122514e11b5R159)