Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruby 2.7 #891

Merged
merged 4 commits into from
Jan 8, 2020
Merged

Ruby 2.7 #891

merged 4 commits into from
Jan 8, 2020

Conversation

dennisvdvliet
Copy link
Contributor

Took a stab at making the Stripe gem ruby 2.7 friendly.

I was able to get rid of most of the warnings with very little effort (just adding lots of **s).

The following warnings remain:

@ob-stripe
Copy link
Contributor

Nice! Thanks @dennisvdvliet.

@brandur-stripe Mind taking a look?

r? @brandur-stripe

@brandur-stripe
Copy link
Contributor

Thank you Dennis! Looks great.

Caused by the way .update_attributes is implemented on Stripe::Object (https://github.com/dennisvdvliet/stripe-ruby/blob/master/lib/stripe/stripe_object.rb#L147)
Not sure what the best way to fix this is since this part of the public api of the gem I belief. Suggestions welcome.

Is the issue with this one is just that Ruby doesn't like us mixing an options hash (opts = {}) with an optional named parameter (dirty: false)?

If so, one thing we could do is just add a new method that the original update_attributes calls into. Although you're right in that update_attributes is public, the use of dirty should be considered internal-only — it's just needed to initialize objects in the first place.

So something like this:

diff --git lib/stripe/stripe_object.rb lib/stripe/stripe_object.rb
index aec74b1..7e2a9b1 100644
--- lib/stripe/stripe_object.rb
+++ lib/stripe/stripe_object.rb
@@ -127,6 +127,18 @@ module Stripe
         JSON.pretty_generate(@values)
     end
 
+    # Mass assigns attributes on the model.
+    #
+    # ==== Attributes
+    #
+    # * +values+ - Hash of values to use to update the current attributes of
+    #   the object.
+    # * +opts+ - Options for +StripeObject+ like an API key that will be reused
+    #   on subsequent API calls.
+    def update_attributes(values, opts = {})
+      update_attributes_internal(values, opts, dirty: false)
+    end
+
     # Mass assigns attributes on the model.
     #
     # This is a version of +update_attributes+ that takes some extra options
@@ -144,7 +156,7 @@ module Stripe
     # * +:dirty+ - Whether values should be initiated as "dirty" (unsaved) and
     #   which applies only to new StripeObjects being initiated under this
     #   StripeObject. Defaults to true.
-    def update_attributes(values, opts = {}, dirty: true)
+    private def update_attributes_internal(values, opts, dirty: true)
       values.each do |k, v|
         add_accessors([k], values) unless metaclass.method_defined?(k.to_sym)
         @values[k] = Util.convert_to_stripe_object(v, opts)
@@ -427,7 +439,7 @@ module Stripe
         @unsaved_values.delete(k)
       end
 
-      update_attributes(values, opts, dirty: false)
+      update_attributes_internal(values, opts, dirty: false)
       values.each_key do |k|
         @transient_values.delete(k)
         @unsaved_values.delete(k)

The one problem is that this is one of those frustrating changes that probably isn't breaking, but technically could be considered to be in the strictest sense.

@brandur-stripe
Copy link
Contributor

(And going to merge what you have here for now.)

@brandur-stripe brandur-stripe merged commit 9c49df7 into stripe:master Jan 8, 2020
@brandur-stripe
Copy link
Contributor

Released as 5.13.0.

@dennisvdvliet
Copy link
Contributor Author

dennisvdvliet commented Jan 9, 2020

Is the issue with this one is just that Ruby doesn't like us mixing an options hash (opts = {}) with an optional named parameter (dirty: false)?

Yes, more details here: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

I found an alternative approach where we explicitly pass in the values as a hash in the tests. This is non breaking for people that never want to upgrade to 2.7 and also how the maintainers of ruby suggest you go about this (see the "Typical cases" paragraph in the page linked above).

See: #892

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants