search

urbanairship / ruby-library

A Ruby wrapper for the Urban Airship API.
Other
200 stars 118 forks source link

Only use Ruby extensions if ActiveSupport missing #81

Closed bswinnerton closed 9 years ago

bswinnerton commented 9 years ago

Overriding ActiveSupport's Object#try and Hash#Compact was surprising behavior to find inside a gem. It's possible that when in a Rails environment, the load order will use this gem's implementation of the two methods rather than the well tested implementations found in ActiveSupport.

This commit proposes only defining these extensions in the event that the methods have not already been defined in something like ActiveSupport.

I would strongly recommend at the very least using the same method signature and implementation that has been defined in ActiveSupport for Object#try and Hash#compact. Perhaps that can be done in a subsequent pull request.

arches commented 9 years ago

+1

dogweather commented 9 years ago

:+1: Yeah, I agree.

jkvoorhis commented 9 years ago

@bswinnerton thank you for putting together this PR. We will be reviewing it in the coming days, and in the meantime could you please sign and submit Urban Airship's contribution agreement?

hartator commented 9 years ago

+1. We are getting the same issue in our rails application.

It seems to not play well with this line of ActionPack: expand_backtrace if exception.is_a?(SyntaxError) || exception.try(:original_exception).try(:is_a?, SyntaxError)

Ref: https://github.com/rails/docrails/blob/master/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb#L40

dogweather commented 9 years ago

FYI @jkvoorhis an option is to remove these altogether and just uglify the code a little. Hash#compact is just pure luxury: it can easily be a function which takes a hash as a parameter instead. Removing Object#try may mean some uglier if/then branching, but the pay-off would be not worrying about collisions with other libraries.

jkvoorhis commented 9 years ago

@dogweather thank you for your input and the initial contributions you made to this codebase. We are currently deciding within the organization what course of action we would like to take regarding these compatibility issues. We will have an update about our decision soon.

bpaul commented 9 years ago

:+1: on removing these. I was really surprised to find these sort of overrides in the gem. Our rails app won't even start after upgrading the gem...

bswinnerton commented 9 years ago

Also a :+1: for flat out removing these altogether. I just wanted to stress the timeliness of this implementation breaking bug with the deprecation of your V1 API in two days. Happy to help in any way possible to expedite this fix.

jkvoorhis commented 9 years ago

We have put out a release that should resolve these conflicts. Version 3.0.2 of our gem removes the methods that were conflicting with ActiveSupport in favor of re-casting them as helper methods. Thank you for your contributions, and please let us know if any other issues arise.