use `alias_method_chain` rather than overriding `render` directly #1

Merged
merged 2 commits into from Dec 3, 2013

Conversation

Projects
None yet
2 participants
@ghiculescu
Contributor

ghiculescu commented Aug 12, 2013

I noticed this wasn't playing nicely with wicked_pdf, which uses alias_method_chain to give different behaviour to the render method. FlashRender wasn't getting called as as result. Adding alias_method_chain to this gem fixes the issue (swapping the order of the gems in my Gemfile also worked, but this seems like a better way).

@ghiculescu

This comment has been minimized.

Show comment
Hide comment
@ghiculescu

ghiculescu Nov 29, 2013

Contributor

Sorry to bug @adamhunter , just wondering if you had a chance to look at this?

Contributor

ghiculescu commented Nov 29, 2013

Sorry to bug @adamhunter , just wondering if you had a chance to look at this?

@adamhunter

This comment has been minimized.

Show comment
Hide comment
@adamhunter

adamhunter Dec 2, 2013

Owner

hey, sorry was traveling for holiday. looking now.

Owner

adamhunter commented Dec 2, 2013

hey, sorry was traveling for holiday. looking now.

lib/flash_render.rb
+ def self.included(base)
+ # Protect from trying to augment modules that appear
+ # as the result of adding other gems.
+ return if base != ActionController::Base

This comment has been minimized.

@adamhunter

adamhunter Dec 2, 2013

Owner

this could be clearer as return unless base == ActionController::Base

@adamhunter

adamhunter Dec 2, 2013

Owner

this could be clearer as return unless base == ActionController::Base

lib/flash_render.rb
+ # as the result of adding other gems.
+ return if base != ActionController::Base
+
+ base.class_eval do

This comment has been minimized.

@adamhunter

adamhunter Dec 2, 2013

Owner

do you have to call class_eval here? wouldn't base.alias_method_chain :render, :flash work, or base.send?

@adamhunter

adamhunter Dec 2, 2013

Owner

do you have to call class_eval here? wouldn't base.alias_method_chain :render, :flash work, or base.send?

@adamhunter

This comment has been minimized.

Show comment
Hide comment
@adamhunter

adamhunter Dec 2, 2013

Owner

made a few comments, looks good overall. i'll merge and push a new version once we've figured out the specifics.

Owner

adamhunter commented Dec 2, 2013

made a few comments, looks good overall. i'll merge and push a new version once we've figured out the specifics.

@ghiculescu

This comment has been minimized.

Show comment
Hide comment
@ghiculescu

ghiculescu Dec 2, 2013

Contributor

Thanks @adamhunter, I've updated based on those comments.

Contributor

ghiculescu commented Dec 2, 2013

Thanks @adamhunter, I've updated based on those comments.

adamhunter added a commit that referenced this pull request Dec 3, 2013

Merge pull request #1 from ghiculescu/master
use `alias_method_chain` rather than overriding `render` directly

@adamhunter adamhunter merged commit c907a18 into adamhunter:master Dec 3, 2013

@adamhunter

This comment has been minimized.

Show comment
Hide comment
@adamhunter

adamhunter Dec 3, 2013

Owner

release 1.2.0. thanks again!

Owner

adamhunter commented Dec 3, 2013

release 1.2.0. thanks again!

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