Update Pagy:: Backend comment
#745
Conversation
gem/lib/pagy/backend.rb
Outdated
| @@ -3,7 +3,7 @@ | |||
|
|
|||
| class Pagy | |||
| # Define a few generic methods to paginate a collection out of the box, | |||
| # or any collection by overriding pagy_get_items and/or pagy_get_vars in your controller | |||
| # or any collection by overriding pagy_get_items and/or pagy_get_count in your controller | |||
There was a problem hiding this comment.
Nicely spotted!
- pagy_get_vars
+ pagy_get_countAny of the pagy_get_ methods may be overridden (for whatever reason): should we alert users to this possibility rather than confine the patch to just pagy_get_items and pagy_get_count?
There was a problem hiding this comment.
Any of the
pagy_get_*methods...
👍🏼 @benkoshy since it's a code comment... yes, a general comment may be more appropriate.
We should consider also adapting it to the backend extras.
There was a problem hiding this comment.
@benkoshy Ah yes fair enough. I wasn't sure so I went for the least changes.
Ive updated the comment to:
# Define a few generic methods to paginate a collection out of the box,
# or any collection by overriding any of the `pagy_get_` methods in your controller.
# See also the extras if you need specialized methods to paginate Arrays or other collectionsDo you think that's better? Or do you have some better suggestions?
There was a problem hiding this comment.
Do you think that's better? Or do you have some better suggestions?
Looks great!
BTW: if you want bonus marks - consider adapting a similar comment in the "extras". i.e. to alert to the possibility that those methods can be overridden. Or I can do it for you and credit you with the PR.
There was a problem hiding this comment.
consider adapting a similar comment in the "extras".
Hm, are you talking about the Pagy::PagyExtra?
I've never overridden those and I'm not sure if I can provide a clear comment instruction on how to override them.
Seems like you need make some extra steps to override frontend methods.
I would be happy if someone else with actual experience can provide a comment there.
There was a problem hiding this comment.
Hm, are you talking about the Pagy::PagyExtra?
Pagy has some "extras" with methods that be overriden. A comment similar to what you suggested above would suffice?
There was a problem hiding this comment.
I have updated with a note suggesting that backend methods can be overridden and have credited Leon Vogt
if you are satisfied i can squate / rebase on master?
There was a problem hiding this comment.
Hi @benkoshy, sorry for my late response! Yes that would be great. I would appreciate it, if you could add the missing comments.
I currently don't have much time. I've rebased this branch, in case you want to use it. Or of course feel free to discard this.
Thank you!
|
This PR requires a rebase on master in order to pass the tests. |
leonvogt
force-pushed
the
update-backend-comment
branch
from
ae11c2d
to
baa1c7c
Compare
leonvogt
force-pushed
the
update-backend-comment
branch
from
baa1c7c
to
5b04f2b
Compare
Hi 👋
I've noticed that the overriding of
pagy_get_varsstopped working after the 9.0.0 upgrade.It seems this method was removed during this refactoring.
In my use case, I was able to switch to overriding
pagy_get_countinstead.I've updated the comment to reflect this change.