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

Respect pagination_options[:renderer] #176

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ghost
Copy link

ghost commented Sep 6, 2011

Hey,

This is related to the pull request I made earlier, it respects pagination_options[:renderer] for all frameworks(Sinatra, Merb, Rails). I added tests for the ActionView changes, and I couldn't find any tests relating to Sinatra/Merb. Since the changes are identical, i think it should be okay to rely on the ActionView tests, but let me know if you'd like me to add tests for Sinatra/Merb.

Cheers,
Rob

@ghost

This comment has been minimized.

Copy link

ghost commented Sep 6, 2011

I noticed a problem with these specs, gonna update them now.

@ghost

This comment has been minimized.

Copy link

ghost commented Sep 6, 2011

I rewrote the specs. I'm really unfamiliar with Mocha, but I think I got it setup properly now.
Appreciate if you can have a look :)

@phene

This comment has been minimized.

Copy link
Contributor

phene commented Sep 8, 2011

+1 Looks good.

@mislav

This comment has been minimized.

Copy link
Owner

mislav commented Sep 15, 2011

Hey @robgleeson,

There are few things wrong with your patch. First, you're not guarding yourself from editing options objects passed in from outside to the will_paginate helpers. You're writing the :renderer key to them in Merb in Sinatra. Second, the tests are too brittle. I know you have gone through much trouble to mock this up with Mocha but there easier ways of doing this without mocking, and the tests could also check if the correct renderer was really used for the HTML output (which your tests don't do).

But, since the last time I've talked I have thought a lot about this and related issues and I realized I can't encourage people to set a global link renderer. This is because of Rack apps mounted inside of each another, like Sinatra inside Rails. If a Rails user set a global link renderer that works with Rails url helpers, that link renderer would implicitly be used in Sinatra and break. The proper way of setting a default link renderer for your application would be wrapping the will_paginate helper with your own and setting the renderer there.

So, I'm going to change the code to deprecate the global option and close this. Sorry about wasting your time, but even I didn't predict these problems.

@mislav mislav closed this Sep 15, 2011

mislav added a commit that referenced this pull request Sep 15, 2011

remove :renderer from global pagination_options to discourage using it
Because lately it's popular to mount different web frameworks in the
same Ruby process (e.g. Sinatra apps beside Rails), a global link
renderer option is useless since it would affect all frameworks but it's
hardly possible that a single renderer will work in all of these
frameworks.

See resolution for #176
@ghost

This comment has been minimized.

Copy link

ghost commented Sep 15, 2011

No problem!

Yeah, I thought a global renderer might cause conflicts within Rails with mounted Rack applications, too.
Thanks for the response.

  • Rob
@korny

This comment has been minimized.

Copy link

korny commented Mar 1, 2012

The proper way of setting a default link renderer for your application would be wrapping the will_paginate helper with your own and setting the renderer there.

I agree! This should be mentioned in the warning.

@banesto

This comment has been minimized.

Copy link

banesto commented Jan 22, 2013

Maybe someone could provide an example how to override will_paginate function in helper?

@mislav

This comment has been minimized.

Copy link
Owner

mislav commented Jan 22, 2013

@banesto

This comment has been minimized.

Copy link

banesto commented Jan 23, 2013

@mislav Thanks a lot!

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