Fix low hanging Rubocop TODOs

I wanted to see what fixing Rubocop TODOs was like, so I tried to
eliminate all the easy ones. Most of these were pretty easy, and the
changes required are relatively minimal.

Some of the stuff left is harder. Pretty much everything under
`Metrics/*` is going to be a pretty big yak shave. A few of the others
are just going to need a little more work (e.g. `Style/ClassVars` and
`Style/GuardClause`). Going to stop here for now.

.rubocop_todo.yml

@@ -6,74 +6,6 @@
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
-# Offense count: 1
-# Configuration parameters: Include.
-# Include: **/Gemfile, **/gems.rb
-Bundler/DuplicatedGem:
-  Exclude:
-    - 'Gemfile'
-
-# Offense count: 1
-Lint/AmbiguousRegexpLiteral:
-  Exclude:
-    - 'test/stripe/stripe_object_test.rb'
-
-# Offense count: 3
-# Configuration parameters: AllowSafeAssignment.
-Lint/AssignmentInCondition:
-  Exclude:
-    - 'lib/stripe/account.rb'
-    - 'lib/stripe/api_resource.rb'
-
-# Offense count: 1
-# Cop supports --auto-correct.
-# Configuration parameters: EnforcedStyleAlignWith, SupportedStylesAlignWith, AutoCorrect.
-# SupportedStylesAlignWith: keyword, variable, start_of_line
-Lint/EndAlignment:
-  Exclude:
-    - 'lib/stripe/stripe_client.rb'
-
-# Offense count: 2
-Lint/ImplicitStringConcatenation:
-  Exclude:
-    - 'test/stripe/webhook_test.rb'
-
-# Offense count: 3
-Lint/IneffectiveAccessModifier:
-  Exclude:
-    - 'lib/stripe.rb'
-    - 'lib/stripe/invoice.rb'
-    - 'lib/stripe/stripe_object.rb'
-
-# Offense count: 1
-Lint/LiteralInCondition:
-  Exclude:
-    - 'lib/stripe/stripe_object.rb'
-
-# Offense count: 2
-Lint/NonLocalExitFromIterator:
-  Exclude:
-    - 'lib/stripe/util.rb'
-
-# Offense count: 5
-Lint/RescueWithoutErrorClass:
-  Exclude:
-    - 'lib/stripe/stripe_client.rb'
-    - 'lib/stripe/util.rb'
-    - 'lib/stripe/webhook.rb'
-
-# Offense count: 2
-# Configuration parameters: ContextCreatingMethods, MethodCreatingMethods.
-Lint/UselessAccessModifier:
-  Exclude:
-    - 'lib/stripe.rb'
-    - 'lib/stripe/util.rb'
-
-# Offense count: 1
-Lint/UselessAssignment:
-  Exclude:
-    - 'stripe.gemspec'
-
 # Offense count: 18
 Metrics/AbcSize:
   Max: 49
@@ -116,19 +48,7 @@ Metrics/ParameterLists:
 # Offense count: 5
 Metrics/PerceivedComplexity:
   Max: 11
-
-# Offense count: 4
-Naming/AccessorMethodName:
   Exclude:
-    - 'lib/stripe/stripe_client.rb'
-    - 'test/stripe/api_resource_test.rb'
-
-# Offense count: 3
-# Cop supports --auto-correct.
-# Configuration parameters: AutoCorrect.
-Performance/HashEachMethods:
-  Exclude:
-    - 'lib/stripe/api_operations/save.rb'
     - 'lib/stripe/stripe_object.rb'
 
 # Offense count: 1
@@ -136,13 +56,6 @@ Security/MarshalLoad:
   Exclude:
     - 'lib/stripe/stripe_object.rb'
 
-# Offense count: 1
-# Configuration parameters: EnforcedStyle, SupportedStyles.
-# SupportedStyles: nested, compact
-Style/ClassAndModuleChildren:
-  Exclude:
-    - 'test/test_helper.rb'
-
 # Offense count: 2
 Style/ClassVars:
   Exclude:
@@ -153,11 +66,6 @@ Style/ClassVars:
 Style/Documentation:
   Enabled: false
 
-# Offense count: 7
-Style/DoubleNegation:
-  Exclude:
-    - 'test/stripe/stripe_client_test.rb'
-
 # Offense count: 5
 # Configuration parameters: MinBodyLength.
 Style/GuardClause:
@@ -166,17 +74,3 @@ Style/GuardClause:
     - 'lib/stripe/source.rb'
     - 'lib/stripe/stripe_client.rb'
     - 'lib/stripe/stripe_object.rb'
-
-# Offense count: 1
-Style/IfInsideElse:
-  Exclude:
-    - 'lib/stripe/stripe_object.rb'
-
-# Offense count: 2
-# Cop supports --auto-correct.
-# Configuration parameters: AutoCorrect, EnforcedStyle, SupportedStyles.
-# SupportedStyles: predicate, comparison
-Style/NumericPredicate:
-  Exclude:
-    - 'spec/**/*'
-    - 'lib/stripe/util.rb'

Gemfile

@@ -23,7 +23,7 @@ group :development do
   if RUBY_VERSION >= "2.2.2"
     gem "rack", ">= 1.5"
   else
-    gem "rack", ">= 1.5", "< 2.0"
+    gem "rack", ">= 1.5", "< 2.0" # rubocop:disable Bundler/DuplicatedGem
   end
 
   platforms :mri do

lib/stripe.rb

@@ -4,6 +4,7 @@ require "cgi"
 require "logger"
 require "openssl"
 require "rbconfig"
+require "securerandom"
 require "set"
 require "socket"
 
@@ -212,12 +213,11 @@ module Stripe
     }
   end
 
-  private
-
   # DEPRECATED. Use `Util#encode_parameters` instead.
   def self.uri_encode(params)
     Util.encode_parameters(params)
   end
+  private_class_method :uri_encode
   class << self
     extend Gem::Deprecate
     deprecate :uri_encode, "Stripe::Util#encode_parameters", 2016, 1

lib/stripe/account.rb

@@ -71,8 +71,8 @@ module Stripe
     end
 
     def serialize_params_account(_obj, update_hash)
-      if entity = @values[:legal_entity]
+      if (entity = @values[:legal_entity])
-        if owners = entity[:additional_owners]
+        if (owners = entity[:additional_owners])
           entity_update = update_hash[:legal_entity] ||= {}
           entity_update[:additional_owners] =
             serialize_additional_owners(entity, owners)

lib/stripe/api_operations/save.rb

@@ -15,7 +15,7 @@ module Stripe
         #   idempotency_key to be passed in the request headers, or for the
         #   api_key to be overwritten. See {APIOperations::Request.request}.
         def update(id, params = {}, opts = {})
-          params.each do |k, _v|
+          params.each_key do |k|
             if protected_fields.include?(k)
               raise ArgumentError, "Cannot update protected field: #{k}"
             end

lib/stripe/api_resource.rb

@@ -45,7 +45,7 @@ module Stripe
     end
 
     def resource_url
-      unless id = self["id"]
+      unless (id = self["id"])
         raise InvalidRequestError.new("Could not determine which URL to request: #{self.class} instance has invalid ID: #{id.inspect}", "id")
       end
       "#{self.class.resource_url}/#{CGI.escape(id)}"

lib/stripe/invoice.rb

@@ -16,14 +16,14 @@ module Stripe
       initialize_from(resp.data, opts)
     end
 
-    private
-
     def self.upcoming_url
       resource_url + "/upcoming"
     end
+    private_class_method :upcoming_url
 
     def pay_url
       resource_url + "/pay"
     end
+    private :pay_url
   end
 end

lib/stripe/stripe_client.rb

@@ -201,7 +201,7 @@ module Stripe
       # We rescue all exceptions from a request so that we have an easy spot to
       # implement our retry logic across the board. We'll re-raise if it's a type
       # of exception that we didn't expect to handle.
-      rescue => e
+      rescue StandardError => e
         # If we modify context we copy it into a new variable so as not to
         # taint the original on a retry.
         error_context = context
@@ -420,7 +420,7 @@ module Stripe
         headers.update(
           "X-Stripe-Client-User-Agent" => JSON.generate(user_agent)
         )
-      rescue => e
+      rescue StandardError => e
         headers.update(
           "X-Stripe-Client-Raw-User-Agent" => user_agent.inspect,
           :error => "#{e} (#{e.class})"
@@ -517,7 +517,7 @@ module Stripe
                     resp.headers
                   else
                     resp[:headers]
-        end
+                  end
 
         context = dup
         context.account = headers["Stripe-Account"]
@@ -532,22 +532,22 @@ module Stripe
     # in so that we can generate a rich user agent header to help debug
     # integrations.
     class SystemProfiler
-      def self.get_uname
+      def self.uname
         if File.exist?("/proc/version")
           File.read("/proc/version").strip
         else
           case RbConfig::CONFIG["host_os"]
           when /linux|darwin|bsd|sunos|solaris|cygwin/i
-            get_uname_from_system
+            uname_from_system
           when /mswin|mingw/i
-            get_uname_from_system_ver
+            uname_from_system_ver
           else
             "unknown platform"
           end
         end
       end
 
-      def self.get_uname_from_system
+      def self.uname_from_system
         (`uname -a 2>/dev/null` || "").strip
       rescue Errno::ENOENT
         "uname executable not found"
@@ -555,7 +555,7 @@ module Stripe
         "uname lookup failed"
       end
 
-      def self.get_uname_from_system_ver
+      def self.uname_from_system_ver
         (`ver` || "").strip
       rescue Errno::ENOENT
         "ver executable not found"
@@ -564,7 +564,7 @@ module Stripe
       end
 
       def initialize
-        @uname = self.class.get_uname
+        @uname = self.class.uname
       end
 
       def user_agent

lib/stripe/stripe_object.rb

@@ -153,7 +153,7 @@ module Stripe
     # values in a tenant array are also marked as dirty.
     def dirty!
       @unsaved_values = Set.new(@values.keys)
-      @values.each do |_k, v|
+      @values.each_value do |v|
         dirty_value!(v)
       end
     end
@@ -194,8 +194,6 @@ module Stripe
       deprecate :serialize_params, "#serialize_params", 2016, 9
     end
 
-    protected
-
     # A protected field is one that doesn't get an accessor assigned to it
     # (i.e. `obj.public = ...`) and one which is not allowed to be updated via
     # the class level `Model.update(id, { ... })`.
@@ -203,6 +201,8 @@ module Stripe
       []
     end
 
+    protected
+
     def metaclass
       class << self; self; end
     end
@@ -271,8 +271,8 @@ module Stripe
           raise NoMethodError, "Cannot set #{attr} on this object. HINT: you can't set: #{@@permanent_attributes.to_a.join(', ')}"
         end
         return mth.call(args[0])
-      else
+      elsif @values.key?(name)
-        return @values[name] if @values.key?(name)
+        return @values[name]
       end
 
       begin
@@ -323,7 +323,7 @@ module Stripe
       end
 
       update_attributes(values, opts, dirty: false)
-      values.each do |k, _|
+      values.each_key do |k|
         @transient_values.delete(k)
         @unsaved_values.delete(k)
       end
@@ -332,8 +332,7 @@ module Stripe
     end
 
     def serialize_params_value(value, original, unsaved, force, key: nil)
-      case true
+      if value.nil?
-      when value.nil?
         ""
 
       # The logic here is that essentially any object embedded in another
@@ -358,7 +357,7 @@ module Stripe
       # We throw an error if a property was set explicitly but we can't do
       # anything with it because the integration is probably not working as the
       # user intended it to.
-      when value.is_a?(APIResource) && !value.save_with_parent
+      elsif value.is_a?(APIResource) && !value.save_with_parent
         if !unsaved
           nil
         elsif value.respond_to?(:id) && !value.id.nil?
@@ -369,7 +368,7 @@ module Stripe
             "not marked as `save_with_parent`."
         end
 
-      when value.is_a?(Array)
+      elsif value.is_a?(Array)
         update = value.map { |v| serialize_params_value(v, nil, true, force) }
 
         # This prevents an array that's unchanged from being resent.
@@ -385,10 +384,10 @@ module Stripe
       # existing array being held by a StripeObject. This could happen for
       # example by appending a new hash onto `additional_owners` for an
       # account.
-      when value.is_a?(Hash)
+      elsif value.is_a?(Hash)
         Util.convert_to_stripe_object(value, @opts).serialize_params
 
-      when value.is_a?(StripeObject)
+      elsif value.is_a?(StripeObject)
         update = value.serialize_params(force: force)
 
         # If the entire object was replaced, then we need blank each field of

lib/stripe/util.rb

@@ -119,7 +119,7 @@ module Stripe
       # (such as AFS)
 
       File.open(file) { |f| }
-    rescue
+    rescue StandardError
       false
     else
       true
@@ -132,7 +132,7 @@ module Stripe
         object.each do |key, value|
           key = (begin
                    key.to_sym
-                 rescue
+                 rescue StandardError
                    key
                  end) || key
           new_hash[key] = symbolize_names(value)
@@ -281,10 +281,12 @@ module Stripe
 
       res = 0
       b.each_byte { |byte| res |= byte ^ l.shift }
-      res == 0
+      res.zero?
     end
 
-    private
+    #
+    # private
+    #
 
     COLOR_CODES = {
       black:   0, light_black:   60,
@@ -319,8 +321,8 @@ module Stripe
     def self.check_array_of_maps_start_keys!(arr)
       expected_key = nil
       arr.each do |item|
-        return unless item.is_a?(Hash)
+        break unless item.is_a?(Hash)
-        return if item.count == 0
+        break if item.count.zero?
 
         first_key = item.first[0]
 

lib/stripe/webhook.rb

@@ -49,7 +49,7 @@ module Stripe
       def self.verify_header(payload, header, secret, tolerance: nil)
         begin
           timestamp, signatures = get_timestamp_and_signatures(header, EXPECTED_SCHEME)
-        rescue
+        rescue StandardError
           raise SignatureVerificationError.new(
             "Unable to extract timestamp and signatures from header",
             header, http_body: payload

stripe.gemspec

@@ -2,7 +2,7 @@ $LOAD_PATH.unshift(File.join(File.dirname(__FILE__), "lib"))
 
 require "stripe/version"
 
-spec = Gem::Specification.new do |s|
+Gem::Specification.new do |s|
   s.name = "stripe"
   s.version = Stripe::VERSION
   s.required_ruby_version = ">= 2.0.0"

test/stripe/api_resource_test.rb

@@ -490,10 +490,10 @@ module Stripe
     @@fixtures = {}
     setup do
       if @@fixtures.empty?
-        set_fixture(:charge) do
+        cache_fixture(:charge) do
           Charge.retrieve("ch_123")
         end
-        set_fixture(:customer) do
+        cache_fixture(:customer) do
           Customer.retrieve("cus_123")
         end
       end
@@ -509,13 +509,9 @@ module Stripe
       @@fixtures[:customer]
     end
 
-    def get_fixture(key)
-      @@fixtures.fetch(key)
-    end
-
     # Expects to retrieve a fixture from stripe-mock (an API call should be
     # included in the block to yield to) and does very simple memoization.
-    def set_fixture(key)
+    def cache_fixture(key)
       return @@fixtures[key] if @@fixtures.key?(key)
 
       obj = yield

test/stripe/stripe_client_test.rb

@@ -409,7 +409,6 @@ module Stripe
             client.execute_request(:post, "/v1/charges")
           rescue Stripe::InvalidRequestError => e
             assert_equal(400, e.http_status)
-            assert_equal(true, !!e.http_body)
             assert_equal(true, e.json_body.is_a?(Hash))
           end
         end
@@ -422,7 +421,6 @@ module Stripe
             client.execute_request(:post, "/v1/charges")
           rescue Stripe::AuthenticationError => e
             assert_equal(401, e.http_status)
-            assert_equal(true, !!e.http_body)
             assert_equal(true, e.json_body.is_a?(Hash))
           end
         end
@@ -435,7 +433,6 @@ module Stripe
             client.execute_request(:post, "/v1/charges")
           rescue Stripe::CardError => e
             assert_equal(402, e.http_status)
-            assert_equal(true, !!e.http_body)
             assert_equal(true, e.json_body.is_a?(Hash))
           end
         end
@@ -448,7 +445,6 @@ module Stripe
             client.execute_request(:post, "/v1/charges")
           rescue Stripe::PermissionError => e
             assert_equal(403, e.http_status)
-            assert_equal(true, !!e.http_body)
             assert_equal(true, e.json_body.is_a?(Hash))
           end
         end
@@ -461,7 +457,6 @@ module Stripe
             client.execute_request(:post, "/v1/charges")
           rescue Stripe::InvalidRequestError => e
             assert_equal(404, e.http_status)
-            assert_equal(true, !!e.http_body)
             assert_equal(true, e.json_body.is_a?(Hash))
           end
         end
@@ -474,7 +469,6 @@ module Stripe
             client.execute_request(:post, "/v1/charges")
           rescue Stripe::RateLimitError => e
             assert_equal(429, e.http_status)
-            assert_equal(true, !!e.http_body)
             assert_equal(true, e.json_body.is_a?(Hash))
           end
         end
@@ -491,7 +485,6 @@ module Stripe
           end
 
           assert_equal(400, e.http_status)
-          assert_equal(true, !!e.http_body)
           assert_equal("No grant type specified", e.message)
         end
 
@@ -699,26 +692,26 @@ module Stripe
   end
 
   class SystemProfilerTest < Test::Unit::TestCase
-    context "#get_uname" do
+    context "#uname" do
       should "run without failure" do
         # Don't actually check the result because we try a variety of different
         # strategies that will have different results depending on where this
         # test and running. We're mostly making sure that no exception is thrown.
-        _ = StripeClient::SystemProfiler.get_uname
+        _ = StripeClient::SystemProfiler.uname
       end
     end
 
-    context "#get_uname_from_system" do
+    context "#uname_from_system" do
       should "run without failure" do
         # as above, just verify that an exception is not thrown
-        _ = StripeClient::SystemProfiler.get_uname_from_system
+        _ = StripeClient::SystemProfiler.uname_from_system
       end
     end
 
-    context "#get_uname_from_system_ver" do
+    context "#uname_from_system_ver" do
       should "run without failure" do
         # as above, just verify that an exception is not thrown
-        _ = StripeClient::SystemProfiler.get_uname_from_system_ver
+        _ = StripeClient::SystemProfiler.uname_from_system_ver
       end
     end
   end

test/stripe/stripe_object_test.rb

@@ -352,7 +352,7 @@ module Stripe
       e = assert_raises ArgumentError do
         obj.foo = ""
       end
-      assert_match /\(object\).foo = nil/, e.message
+      assert_match(/\(object\).foo = nil/, e.message)
     end
   end
 end

test/stripe/webhook_test.rb

@@ -2,10 +2,12 @@ require File.expand_path("../../test_helper", __FILE__)
 
 module Stripe
   class WebhookTest < Test::Unit::TestCase
-    EVENT_PAYLOAD = ""'{
+    EVENT_PAYLOAD = <<-PAYLOAD.freeze
-  "id": "evt_test_webhook",
+      {
-  "object": "event"
+        "id": "evt_test_webhook",
-}'"".freeze
+        "object": "event"
+      }
+    PAYLOAD
     SECRET = "whsec_test_secret".freeze
 
     def generate_header(opts = {})

test/test_helper.rb

@@ -34,23 +34,27 @@ rescue Faraday::ConnectionFailed
     "it running? Please see README for setup instructions.")
 end
 
-class Test::Unit::TestCase
+module Test
-  include Stripe::TestData
+  module Unit
-  include Mocha
+    class TestCase
+      include Stripe::TestData
+      include Mocha
 
-  setup do
+      setup do
-    Stripe.api_key = "sk_test_123"
+        Stripe.api_key = "sk_test_123"
-    Stripe.api_base = "http://localhost:#{MOCK_PORT}"
+        Stripe.api_base = "http://localhost:#{MOCK_PORT}"
-    stub_connect
+        stub_connect
-  end
+      end
 
-  teardown do
+      teardown do
-    Stripe.api_key = nil
+        Stripe.api_key = nil
-  end
+      end
 
-  private
+      private
 
-  def stub_connect
+      def stub_connect
-    stub_request(:any, /^#{Stripe.connect_base}/).to_return(body: "{}")
+        stub_request(:any, /^#{Stripe.connect_base}/).to_return(body: "{}")
+      end
+    end
   end
 end