Update the wiki to support all possible examples that are good. #269

Closed
envygeeks opened this Issue Sep 26, 2012 · 9 comments

Comments

Projects
None yet
2 participants
@envygeeks
# Considering that Will_Paginate is NOT elegent about recieving 0
# Always try to address this issue and never send params[:page]
# directly in, unless you like bad-cases then by all means do it.

page = params[:page].to_i
if page == 0
  page = 1
end

Content.where(:role => :page).paginate(:page => page)
Content.paginate(:page => page).where(:role => :page)
Content.paginate(:page => page).find_all_by_role(:page)

The problem I have with the current situation isn't that it's not right it's that it does not imply that users can infact use finder methods with will_paginate... they just need to reorder the chain and start with paginate.

Actually I would personally imply in the docs that one should always begin with Model.paginate and then do all their other normal queries which will allow them to do pretty much everything they want. So perhaps that is a better idea.

@mislav

This comment has been minimized.

Show comment Hide comment
@mislav

mislav Jan 10, 2013

Owner

will_paginate deliberately raises exceptions for invalid page numbers. You're then given a chance how you want to handle these exceptions. You can do it in the manner described above, yes. But I wouldn't recommend everyone to do that, since very few people actually need it. Most people are happy just passing in page(params[:page]).

Owner

mislav commented Jan 10, 2013

will_paginate deliberately raises exceptions for invalid page numbers. You're then given a chance how you want to handle these exceptions. You can do it in the manner described above, yes. But I wouldn't recommend everyone to do that, since very few people actually need it. Most people are happy just passing in page(params[:page]).

@mislav mislav closed this Jan 10, 2013

@envygeeks

This comment has been minimized.

Show comment Hide comment
@envygeeks

envygeeks Jan 10, 2013

@mislav Come again for big fudge? That had nothing to do with exceptions, or asking you to adjust exceptions or anything else, it had to do with implying in the docs that when querying they need to properly order it and asking that you mention such situations in the documentation.

Are you saying that documentation should be deliberately left broad in that aspect?

@mislav Come again for big fudge? That had nothing to do with exceptions, or asking you to adjust exceptions or anything else, it had to do with implying in the docs that when querying they need to properly order it and asking that you mention such situations in the documentation.

Are you saying that documentation should be deliberately left broad in that aspect?

@mislav

This comment has been minimized.

Show comment Hide comment
@mislav

mislav Jan 10, 2013

Owner

Seems like I've not understood your original proposal.

Why is the ordering of the chain important, and why should you avoid passing in params[:page] directly?

Owner

mislav commented Jan 10, 2013

Seems like I've not understood your original proposal.

Why is the ordering of the chain important, and why should you avoid passing in params[:page] directly?

@envygeeks

This comment has been minimized.

Show comment Hide comment
@envygeeks

envygeeks Jan 10, 2013

@mislav It's nothing to do with the params[:page] that's not important (aside from the part I would never pass a param directly into anything without first checking it.) The part I would like to see the docs clarify is the ordering of chaining. Where Content.where.paginate might cause an error (at least it did back then I haven't played with that code since I posted this) but Content.paginate.where did not cause an error and built a proper ActiveRecord query. The Docs never implied any such thing so it caught me off guard (and while an easy fix it could have been avoided with documentation that clarified that fact in it's text.)

Edit: As a side note since I never worked with Rails 3.1 only 2x and 3.2x I was on 3.2 when I ran into this.

@mislav It's nothing to do with the params[:page] that's not important (aside from the part I would never pass a param directly into anything without first checking it.) The part I would like to see the docs clarify is the ordering of chaining. Where Content.where.paginate might cause an error (at least it did back then I haven't played with that code since I posted this) but Content.paginate.where did not cause an error and built a proper ActiveRecord query. The Docs never implied any such thing so it caught me off guard (and while an easy fix it could have been avoided with documentation that clarified that fact in it's text.)

Edit: As a side note since I never worked with Rails 3.1 only 2x and 3.2x I was on 3.2 when I ran into this.

@mislav

This comment has been minimized.

Show comment Hide comment
@mislav

mislav Jan 10, 2013

Owner

Oh, the ordering of the calls in the chain shouldn't matter. where.paginate is equivalent to paginate.where. If you find instances where this is not true, please report it as a bug with steps to reproduce.

Owner

mislav commented Jan 10, 2013

Oh, the ordering of the calls in the chain shouldn't matter. where.paginate is equivalent to paginate.where. If you find instances where this is not true, please report it as a bug with steps to reproduce.

@envygeeks

This comment has been minimized.

Show comment Hide comment
@envygeeks

envygeeks Jan 10, 2013

The snippet are the steps.

The snippet are the steps.

@mislav

This comment has been minimized.

Show comment Hide comment
@mislav

mislav Jan 10, 2013

Owner

OK, but you didn't give me the model's implementation or schema.

All 3 lines that you've presented are equivalent:

Content.where(:role => :page).paginate(:page => 1)
#=> SELECT "contents".* FROM "contents" WHERE "contents"."role" = 'page' LIMIT 30 OFFSET 0
Content.paginate(:page => 1).where(:role => :page)                                                                                                               
#=> SELECT "contents".* FROM "contents" WHERE "contents"."role" = 'page' LIMIT 30 OFFSET 0
Content.paginate(:page => 1).find_all_by_role(:page)
#=> SELECT "contents".* FROM "contents" WHERE "contents"."role" = 'page' LIMIT 30 OFFSET 0

However you can't chain anything after find_all_by_role because that triggers the query and returns an Array instead of AR Relation. This is the remains of AR's legacy API, however, and shouldn't be used for precisely this reason.

Owner

mislav commented Jan 10, 2013

OK, but you didn't give me the model's implementation or schema.

All 3 lines that you've presented are equivalent:

Content.where(:role => :page).paginate(:page => 1)
#=> SELECT "contents".* FROM "contents" WHERE "contents"."role" = 'page' LIMIT 30 OFFSET 0
Content.paginate(:page => 1).where(:role => :page)                                                                                                               
#=> SELECT "contents".* FROM "contents" WHERE "contents"."role" = 'page' LIMIT 30 OFFSET 0
Content.paginate(:page => 1).find_all_by_role(:page)
#=> SELECT "contents".* FROM "contents" WHERE "contents"."role" = 'page' LIMIT 30 OFFSET 0

However you can't chain anything after find_all_by_role because that triggers the query and returns an Array instead of AR Relation. This is the remains of AR's legacy API, however, and shouldn't be used for precisely this reason.

@envygeeks

This comment has been minimized.

Show comment Hide comment
@envygeeks

envygeeks Jan 10, 2013

Why would I give you my schema information? You don't need that. You would never need that for documentation requests. And just because you wouldn't do something doesn't mean other people won't. I personally think it should be documented that if they chose to use finders with paginate that they should put pagination first, but that conclusion could never be reached because you are treating a documentation request as if it's a bug when it's not and you already rejected the documentation request when you closed the ticket. I honestly don't know why I replied to a loaded close knowing that a close is a rejection so I'm sorry I replied.

Good day.

Why would I give you my schema information? You don't need that. You would never need that for documentation requests. And just because you wouldn't do something doesn't mean other people won't. I personally think it should be documented that if they chose to use finders with paginate that they should put pagination first, but that conclusion could never be reached because you are treating a documentation request as if it's a bug when it's not and you already rejected the documentation request when you closed the ticket. I honestly don't know why I replied to a loaded close knowing that a close is a rejection so I'm sorry I replied.

Good day.

@mislav

This comment has been minimized.

Show comment Hide comment
@mislav

mislav Jan 10, 2013

Owner

You're right, I've closed the ticket too early. I didn't understand what exactly it was that you were explaining. I understand now why you would this behavior to be documented.

However, I don't feel so strongly about documenting that. I don't receive bug reports or support requests about people that have been bitten by trying to do paginate after a find_all_by_* dynamic finders. And also, dynamic finders are deprecated and will be removed in Rails 4. They are the remains of the legacy Active Record API, but nowadays there's not much benefit using them.

Owner

mislav commented Jan 10, 2013

You're right, I've closed the ticket too early. I didn't understand what exactly it was that you were explaining. I understand now why you would this behavior to be documented.

However, I don't feel so strongly about documenting that. I don't receive bug reports or support requests about people that have been bitten by trying to do paginate after a find_all_by_* dynamic finders. And also, dynamic finders are deprecated and will be removed in Rails 4. They are the remains of the legacy Active Record API, but nowadays there's not much benefit using them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment