Refactors code and other minor changes #28

Merged
merged 6 commits into from May 23, 2012

Conversation

Projects
None yet
3 participants
@HashNuke
Contributor

HashNuke commented May 23, 2012

Hi there

This is a re-submit of #27 with changes that were pointed out. (I've closed #27).

Changes:

  • Refactors code into different files. So there's no more one huge stripe.rb
  • Removes rest-client dependency specification from the stripe.rb file. It's already in the gemspec.
  • Requires "stripe" in test_helper.rb instead of the relative path being used.
    Tests now have to be run with bundle exec rake test or equivalent in rbenv. Running without the bundle exec will fail.
  • Adds a "Development" section to the README on how to run the test cases.

Fixes (as pointed out in #27)

  • Rebases commits that aren't important to be in the history.
  • Changes double quotes to single quotes.
    #27 (comment)
  • Changes require path to stripe/version.
    #27 (comment)
  • Fixes typo in commit message.
    #27 (comment)

P.S: In good faith and as a random response to a tweet - https://twitter.com/avdi/status/202091097906888704

HashNuke added some commits May 14, 2012

Moves classes to seperate files
Signed-off-by: Akash Manohar J <akash@akash.im>
Requires all the classes in the stripe.rb file
Signed-off-by: Akash Manohar J <akash@akash.im>
Moves API operations to seperate files
Signed-off-by: Akash Manohar J <akash@akash.im>
@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 23, 2012

This pull request passes (merged 0c27b9c into 35e7375).

This pull request passes (merged 0c27b9c into 35e7375).

@gdb

View changes

lib/stripe.rb
require 'rubygems'
require 'openssl'
-
-gem 'rest-client', '~> 1.4'

This comment has been minimized.

@gdb

gdb May 23, 2012

Contributor

What did you think of my comment on this on the previous pull request (#27 (comment))?

@gdb

gdb May 23, 2012

Contributor

What did you think of my comment on this on the previous pull request (#27 (comment))?

This comment has been minimized.

@HashNuke

HashNuke May 23, 2012

Contributor

Sorry, I forgot to reply to that.

gem() method is provided by Bundler. You can check that from your console itself. Try bundle exec irb, then do method(:gem).source_location. That'll give you the location of the definition.

As per my bundler version (1.1.rc), it's from line-122 of lib/bundler/rubygems_integration.rb

I also did a code search of rubygems/rubygems repo on Github to verify my facts https://github.com/search?q=repo%3Arubygems%2Frubygems+kernel&repo=&langOverride=&start_value=1&type=Code&language=

And it turned out that Rubygems doesn't provide a global gem() method. I checked that rest-client is at v1.6.x, since you are using 1.4 I assume it's because you want 1.8.7 compatibility. Why not just specify that in the gemspec? And also in the readme too. So then anyone who's using the gem from source and without Bundler, will find it easier to install specific versions of dependencies.

@HashNuke

HashNuke May 23, 2012

Contributor

Sorry, I forgot to reply to that.

gem() method is provided by Bundler. You can check that from your console itself. Try bundle exec irb, then do method(:gem).source_location. That'll give you the location of the definition.

As per my bundler version (1.1.rc), it's from line-122 of lib/bundler/rubygems_integration.rb

I also did a code search of rubygems/rubygems repo on Github to verify my facts https://github.com/search?q=repo%3Arubygems%2Frubygems+kernel&repo=&langOverride=&start_value=1&type=Code&language=

And it turned out that Rubygems doesn't provide a global gem() method. I checked that rest-client is at v1.6.x, since you are using 1.4 I assume it's because you want 1.8.7 compatibility. Why not just specify that in the gemspec? And also in the readme too. So then anyone who's using the gem from source and without Bundler, will find it easier to install specific versions of dependencies.

This comment has been minimized.

@HashNuke

HashNuke May 23, 2012

Contributor

It seems like rubygems does provide a gem() method. irb and then method(:gem).source_location.

I would leave it upto you as to what to do now. Should I add that line back if , rebase the commits and send another pull request?

@HashNuke

HashNuke May 23, 2012

Contributor

It seems like rubygems does provide a gem() method. irb and then method(:gem).source_location.

I would leave it upto you as to what to do now. Should I add that line back if , rebase the commits and send another pull request?

This comment has been minimized.

@gdb

gdb May 23, 2012

Contributor

Yep, would add it back, and rebase. You don't actually need to do another pull request -- fine to just 'git push -f' this branch; the pull request will automatically update.

@gdb

gdb May 23, 2012

Contributor

Yep, would add it back, and rebase. You don't actually need to do another pull request -- fine to just 'git push -f' this branch; the pull request will automatically update.

This comment has been minimized.

@gdb

gdb May 23, 2012

Contributor

(Note that the reason we require ~> 1.4 is that pre-1.4 didn't support using your own CA file, or something along those lines. ~> 1.4 allows people to use any version above 1.4 but less than 2.0. We should probably add this to the README.)

@gdb

gdb May 23, 2012

Contributor

(Note that the reason we require ~> 1.4 is that pre-1.4 didn't support using your own CA file, or something along those lines. ~> 1.4 allows people to use any version above 1.4 but less than 2.0. We should probably add this to the README.)

This comment has been minimized.

@HashNuke

HashNuke May 23, 2012

Contributor

Oh I didn't know about that.


Akash Manohar J
http://akash.im
@HashNuke

On Wednesday 23 May 2012 at 11:29 AM, Greg Brockman wrote:

require 'rubygems'

require 'openssl'

-gem 'rest-client', '~> 1.4'

(Note that the reason we require ~> 1.4 is that pre-1.4 didn't support using your own CA file, or something along those lines. ~> 1.4 allows people to use any version above 1.4 but less than 2.0. We should probably add this to the README.)


Reply to this email directly or view it on GitHub:
https://github.com/stripe/stripe-ruby/pull/28/files#r865777

@HashNuke

HashNuke May 23, 2012

Contributor

Oh I didn't know about that.


Akash Manohar J
http://akash.im
@HashNuke

On Wednesday 23 May 2012 at 11:29 AM, Greg Brockman wrote:

require 'rubygems'

require 'openssl'

-gem 'rest-client', '~> 1.4'

(Note that the reason we require ~> 1.4 is that pre-1.4 didn't support using your own CA file, or something along those lines. ~> 1.4 allows people to use any version above 1.4 but less than 2.0. We should probably add this to the README.)


Reply to this email directly or view it on GitHub:
https://github.com/stripe/stripe-ruby/pull/28/files#r865777

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 23, 2012

This pull request passes (merged bae485cd into 35e7375).

This pull request passes (merged bae485cd into 35e7375).

HashNuke added some commits May 14, 2012

Changes order of require
Signed-off-by: Akash Manohar J <akash@akash.im>
Adds note in readme on how to run test cases
Signed-off-by: Akash Manohar J <akash@akash.im>
Requires stripe/version
Signed-off-by: Akash Manohar J <akash@akash.im>
@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 23, 2012

This pull request passes (merged f21c271 into 35e7375).

This pull request passes (merged f21c271 into 35e7375).

@gdb

This comment has been minimized.

Show comment
Hide comment
@gdb

gdb May 23, 2012

Contributor

Awesome, thanks much.

Contributor

gdb commented May 23, 2012

Awesome, thanks much.

@HashNuke

This comment has been minimized.

Show comment
Hide comment
@HashNuke

HashNuke May 23, 2012

Contributor

I'm still having a few doubts, won't the dependency be resolved while installing the gem itself, since the version limit is mentioned in the gemspec?

P.S: You can go ahead and merge it if you feel this is sufficient for now.

Contributor

HashNuke commented May 23, 2012

I'm still having a few doubts, won't the dependency be resolved while installing the gem itself, since the version limit is mentioned in the gemspec?

P.S: You can go ahead and merge it if you feel this is sufficient for now.

gdb added a commit that referenced this pull request May 23, 2012

Merge pull request #28 from HashNuke/refactor2
Refactors code into different files and other minor changes

@gdb gdb merged commit 020177f into stripe:master May 23, 2012

@gdb

This comment has been minimized.

Show comment
Hide comment
@gdb

gdb May 23, 2012

Contributor

That should be the case, yes, for a normal install. However, we've
found that you need to be pretty defensive in client libraries, and
try to handle the cases where invariants end up being broken (because
that happens way more often than you'd like).

  • gdb

On Tue, May 22, 2012 at 11:06 PM, Akash Manohar
reply@reply.github.com
wrote:

I'm still having a few doubts, won't the dependency be resolved while installing the gem itself, since the version limit is mentioned in the gemspec?

P.S: You can go ahead and merge it if you feel this if sufficient for now.


Reply to this email directly or view it on GitHub:
#28 (comment)

Contributor

gdb commented May 23, 2012

That should be the case, yes, for a normal install. However, we've
found that you need to be pretty defensive in client libraries, and
try to handle the cases where invariants end up being broken (because
that happens way more often than you'd like).

  • gdb

On Tue, May 22, 2012 at 11:06 PM, Akash Manohar
reply@reply.github.com
wrote:

I'm still having a few doubts, won't the dependency be resolved while installing the gem itself, since the version limit is mentioned in the gemspec?

P.S: You can go ahead and merge it if you feel this if sufficient for now.


Reply to this email directly or view it on GitHub:
#28 (comment)

@HashNuke HashNuke changed the title from Refactors code into different files and other minor changes to Refactors code and other minor changes Oct 19, 2016

brandur-stripe added a commit that referenced this pull request Apr 5, 2018

Merge pull request #28 from stripe/brandur-update-openapi
Update OpenAPI specification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment