search

bitpay / ruby-client

Powerful, flexible, lightweight SDK for the BitPay Bitcoin Payment Gateway API.
MIT License
79 stars 64 forks source link

Remove unnecessary rack/json dependencies #53

Closed javierjulio closed 8 years ago

javierjulio commented 8 years ago

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.

philosodad commented 8 years ago

Did I mention there is a bitpay-rails gem? Changes to that would also be helpful for use with rails 5.

https://github.com/bitpay/bitpay-rails

On Thu, Aug 4, 2016 at 2:02 PM, Javier Julio notifications@github.com wrote:

Rack isn't used at all so no need to specify. Based on this comment https://github.com/bitpay/ruby-client/pull/51#issuecomment-236736392 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 https://github.com/mperham/sidekiq/issues/2743 to clean up dependencies and also support other platforms (Windows). The json library is being

required in lib/bitpay/client.rb.

You can view, comment on, or merge this pull request online at:

https://github.com/bitpay/ruby-client/pull/53 Commit Summary

  • Remove unnecessary rack/json dependencies

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitpay/ruby-client/pull/53, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIQybgh9UI9_bhq3r4vUoWozzX34k-tks5qcikugaJpZM4Jc9q2 .

J. Paul Daigle

javierjulio commented 8 years ago

@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.

javierjulio commented 8 years ago

@philosodad wanted to see if you had a chance to review my last comment here.

philosodad commented 8 years ago

@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.

javierjulio commented 8 years ago

@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.

philosodad commented 8 years ago

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 wrote:

@philosodad https://github.com/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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitpay/ruby-client/pull/53#issuecomment-239954854, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIQyecD2ohtw9GiFpXnsE3-4i6fXLK0ks5qgO_qgaJpZM4Jc9q2 .

J. Paul Daigle

javierjulio commented 8 years ago

@philosodad I'm so confused. What and how are you seeing that? Do you mean the Gemfile.lock for the bitpay-sdk gem? It lists addressable gem but it has no dependencies. I then checked addressable's gem spec and Gemfile and it does not have a rack dependency either.

philosodad commented 8 years ago

https://github.com/sporkmonger/addressable/blob/master/Gemfile

On Tue, Aug 16, 2016 at 9:00 PM, Javier Julio notifications@github.com wrote:

@philosodad https://github.com/philosodad I'm so confused. What and how are you seeing that? Do you mean the Gemfile.lock for the bitpay-sdk gem? It lists addressable gem but it has no dependencies. I then checked addressable's gem spec https://github.com/sporkmonger/addressable/blob/master/addressable.gemspec and Gemfile and it does not have a rack dependency either.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitpay/ruby-client/pull/53#issuecomment-240284846, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIQyW--bzkEgOHNzuGDVmzx0OS6ogqpks5qgl0YgaJpZM4Jc9q2 .

J. Paul Daigle

philosodad commented 8 years ago

I mean, I'm not positive that that is the source of the issue, but it's the first place I saw rack when I started looking.

On Wed, Aug 17, 2016 at 5:38 PM, Paul Daigle j.paul.daigle@gmail.com wrote:

https://github.com/sporkmonger/addressable/blob/master/Gemfile

On Tue, Aug 16, 2016 at 9:00 PM, Javier Julio notifications@github.com wrote:

@philosodad https://github.com/philosodad I'm so confused. What and how are you seeing that? Do you mean the Gemfile.lock for the bitpay-sdk gem? It lists addressable gem but it has no dependencies. I then checked addressable's gem spec https://github.com/sporkmonger/addressable/blob/master/addressable.gemspec and Gemfile and it does not have a rack dependency either.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitpay/ruby-client/pull/53#issuecomment-240284846, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIQyW--bzkEgOHNzuGDVmzx0OS6ogqpks5qgl0YgaJpZM4Jc9q2 .

J. Paul Daigle

J. Paul Daigle

javierjulio commented 8 years ago

@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.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 91.096% when pulling b789faf87d781c402adad4378da77c7bdcf9b17e on javierjulio:patch-3 into 0ec4bb11056df5fd581834a25aa56c9df1166e25 on bitpay:master.

javierjulio commented 8 years ago

@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?

philosodad commented 8 years ago

Nope. If one of the people at BitPay (probably @kleetus) can have a look I think this is good to go.

kleetus commented 8 years ago

@javierjulio I'll take a look

kleetus commented 8 years ago

Builds are failing because of this issue:

https://github.com/travis-ci/travis-ci/issues/1946

I've looked over the merge request and run the tests locally. This should be good to go.

javierjulio commented 7 years ago

@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!

javierjulio commented 7 years ago

@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.

jarthod commented 6 years ago

It's been a year now, and still no official gem compatible with Rails 5?