Merge pull request #584 from stripe/brandur-rubocop-fixes

Fix low hanging Rubocop TODOs

.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 owners = entity[:additional_owners]
+      if (entity = @values[:legal_entity])
+        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
-        return @values[name] if @values.key?(name)
+      elsif @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
-      when value.nil?
+      if 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)
-        return if item.count == 0
+        break unless item.is_a?(Hash)
+        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 = ""'{
-  "id": "evt_test_webhook",
-  "object": "event"
-}'"".freeze
+    EVENT_PAYLOAD = <<-PAYLOAD.freeze
+      {
+        "id": "evt_test_webhook",
+        "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
-  include Stripe::TestData
-  include Mocha
+module Test
+  module Unit
+    class TestCase
+      include Stripe::TestData
+      include Mocha
 
-  setup do
-    Stripe.api_key = "sk_test_123"
-    Stripe.api_base = "http://localhost:#{MOCK_PORT}"
-    stub_connect
-  end
+      setup do
+        Stripe.api_key = "sk_test_123"
+        Stripe.api_base = "http://localhost:#{MOCK_PORT}"
+        stub_connect
+      end
 
-  teardown do
-    Stripe.api_key = nil
-  end
+      teardown do
+        Stripe.api_key = nil
+      end
 
-  private
+      private
 
-  def stub_connect
-    stub_request(:any, /^#{Stripe.connect_base}/).to_return(body: "{}")
+      def stub_connect
+        stub_request(:any, /^#{Stripe.connect_base}/).to_return(body: "{}")
+      end
+    end
   end
 end