Remove unnecessary rack/json dependencies #53
Conversation
Rack isn't used at all so no need to specify. Based on [this comment](bitpay#51 (comment)) this seems to be carried over from a Rails app so is safe to remove. This will also allow this gem to be used in Rails 5 apps which now require Rack v2. No need to specify json dependency since this gem requires Ruby 2.0+ and I've [seen this change made elsewhere](sidekiq/sidekiq#2743) to clean up dependencies and also support other platforms (Windows). The json library is being required in `lib/bitpay/client.rb`.
|
Did I mention there is a bitpay-rails gem? Changes to that would also be https://github.com/bitpay/bitpay-rails On Thu, Aug 4, 2016 at 2:02 PM, Javier Julio notifications@github.com
J. Paul Daigle |
|
@philosodad do you know why Travis is complaining about rack? Could it be that one of the development dependencies requires it and thus rack should be a development dependency? A coworker suggested that. I can try it, unless you have another idea why. I looked into bitpay-rails but there shouldn't need be any changes there since its just dependent on rails and bitpay-sdk. If we remove the rack dependency here it'll work with Rails 4 and 5 since it already requires rack and the correct version. Not sure if I missed something though. |
|
@philosodad wanted to see if you had a chance to review my last comment here. |
|
@javierjulio the addressable gem requires rack, and it looks like the gem version needs to be pinned to an earlier version or the ruby version needs to be bumped up. |
|
@philosodad I'm having trouble finding where the addressable gem is required? Its not listed in the gemspec as a dependency. Ok, in my first PR I set it to to use Rack 2 if the Ruby version was 2.2.x or higher but still works with Rack 1. Would that ultimately be a better PR than this one? The json dependency can still be removed as stated in the PR description here. |
|
I was just looking at the Gemfile.lock to see what was in it. On Mon, Aug 15, 2016 at 7:02 PM, Javier Julio notifications@github.com
J. Paul Daigle |
|
@philosodad I'm so confused. What and how are you seeing that? Do you mean the |
|
https://github.com/sporkmonger/addressable/blob/master/Gemfile On Tue, Aug 16, 2016 at 9:00 PM, Javier Julio notifications@github.com
J. Paul Daigle |
|
I mean, I'm not positive that that is the source of the issue, but it's the On Wed, Aug 17, 2016 at 5:38 PM, Paul Daigle j.paul.daigle@gmail.com
J. Paul Daigle |
Seems this failing due to addressable but that requires rack-mount but only used in development. Going off a suggestion from a coworker as this seems as an odd dependency to have but most likely should just be set for dev.
|
@philosodad I was thinking the same and had seen that but didn't read carefully enough. It does require rack. I'm going with a suggestion my coworker made that this is probably meant as a development dependency instead. I'll see if the build goes through now without any rack errors. |
|
@philosodad ok that seems to have done it. Just needed to be a development dependency. The errors now are just related to not being able to use encrypted environment variables. Anything else I can do here? Is something else wrong? |
|
Nope. If one of the people at BitPay (probably @kleetus) can have a look I think this is good to go. |
|
@javierjulio I'll take a look |
|
Builds are failing because of this issue: I've looked over the merge request and run the tests locally. This should be good to go. |
|
@kleetus thanks again for doing this. It's been several months though and no other changes in master. We've been installing the gem off master but it would be great to have a proper version instead. Any chance you could do a new release for whats currently in master? Thanks! |
|
@kleetus just wanted to follow, any chance you can cut a new release? Would be useful to have this change without having to install the gem from repo. |
|
It's been a year now, and still no official gem compatible with Rails 5? |
Rack isn't used at all so no need to specify. Based on this comment this seems to be carried over from a Rails app so is safe to remove. This will also allow this gem to be used in Rails 5 apps which now require Rack v2.
No need to specify json dependency since this gem requires Ruby 2.0+ and I've seen this change made elsewhere to clean up dependencies and also support other platforms (Windows). The json library is being required in
lib/bitpay/client.rb.