Fix deprecation warnings

[cgeorgii]

Hi, I wanted to change the after_filter call in lib/wicked_pdf/pdf_helper.rb to make it compliant with the new Rails 5 syntax and ended up also clearing a deprecation warning from running the wicked pdf tests, by changing Mime::JS to Mime[:js] in lib/wicked_pdf/wicked_pdf_helper.rb.

The test suite has a failing test regarding javascript but it was already failing before and I didn't try to implement or change it, but everything else is green.

This is my first open source contribution, and I'm excited to hear some feedback!

[unixmonkey]

Some of the tests in the Travis CI matrix are failing because of your change, because after_action is new in Rails 4, but this project currently aims to support those on Rails versions 2 through 5 (though I wouldn't mind dropping support for 2.x).

An approach that should accommodate everyone would be to wrap it in a conditional like this:

if Rails::VERSION::MAJOR >= 4
  after_action :clean_temp_files
else
  after_filter :clean_temp_files
end

or you could probably use respond_to? (which feels a bit cleaner to me)

if respond_to?(:after_action)
  after_action :clean_temp_files
else
  after_filter :clean_temp_files
end

I have a similar concern about Mime[:js], does that work on older versions of Rails?

Great first contribution though, a couple of tweaks and I'll gladly merge it in!

[cgeorgii]

Weird, even after pinning the rails dependency to 4.2.6 I can't get this
spec to pass (even without my changes).

I didn't realize this change was not backwards compatible, sorry. I will
take another look later this week.

Cheers!

On 09.07.2016 23:14, David Jones wrote:

Some of the tests in the Travis CI matrix
https://travis-ci.org/mileszs/wicked_pdf/jobs/143540764#L1068 are
failing because of your change, because |after_action| is new in Rails
4, but this project currently aims to support those on Rails versions
2 through 5 (though I wouldn't mind dropping support for 2.x).

An approach that should accommodate everyone would be to wrap it in a
conditional like this:

if Rails::VERSION::MAJOR >= 5
after_action:clean_temp_files
else
after_filter:clean_temp_files
end

or you could probably use |respond_to?|

if respond_to?(:after_action)
after_action:clean_temp_files
else
after_filter:clean_temp_files
end

I have a similar concern about Mime[:js], does that work on older
versions of Rails?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#555 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AMEEdDqGKjgEzQpyLXqAq5ajnAedXI6Fks5qUA89gaJpZM4JIl-r.

[unixmonkey]

The change to Mime[:js] in a backwards-compatible way just landed in #627 if you'd like to check it out.
I'd still be interested in your after_filter/after_action switch if you'd be willing to rebase and clean up this PR.

[cgeorgii]

Thanks for letting me know about this. I just fixed the fix applying your suggestion and made a new pull request (since I can't edit this one). #630

[unixmonkey]

Thanks. I merged the other PR. Closing this one.