From 7f85eea3eeceafa4377920f210adbb83818f71f2 Mon Sep 17 00:00:00 2001 From: Brandur Date: Wed, 27 Sep 2017 13:58:20 -0700 Subject: [PATCH] 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 | 106 ------------------------------ Gemfile | 2 +- lib/stripe.rb | 4 +- lib/stripe/account.rb | 4 +- lib/stripe/api_operations/save.rb | 2 +- lib/stripe/api_resource.rb | 2 +- lib/stripe/invoice.rb | 4 +- lib/stripe/stripe_client.rb | 18 ++--- lib/stripe/stripe_object.rb | 23 ++++--- lib/stripe/util.rb | 14 ++-- lib/stripe/webhook.rb | 2 +- stripe.gemspec | 2 +- test/stripe/api_resource_test.rb | 10 +-- test/stripe/stripe_client_test.rb | 19 ++---- test/stripe/stripe_object_test.rb | 2 +- test/stripe/webhook_test.rb | 10 +-- test/test_helper.rb | 32 +++++---- 17 files changed, 73 insertions(+), 183 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 7e2e0076..872297ec 100644 --- a/.rubocop_todo.yml +++ b/.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' diff --git a/Gemfile b/Gemfile index 8283a437..108e5362 100644 --- a/Gemfile +++ b/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 diff --git a/lib/stripe.rb b/lib/stripe.rb index 261b019b..402bf0ea 100644 --- a/lib/stripe.rb +++ b/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 diff --git a/lib/stripe/account.rb b/lib/stripe/account.rb index a7699a0d..f5440bea 100644 --- a/lib/stripe/account.rb +++ b/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) diff --git a/lib/stripe/api_operations/save.rb b/lib/stripe/api_operations/save.rb index f0a04cd5..206a5316 100644 --- a/lib/stripe/api_operations/save.rb +++ b/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 diff --git a/lib/stripe/api_resource.rb b/lib/stripe/api_resource.rb index 9a93e978..75b25c4e 100644 --- a/lib/stripe/api_resource.rb +++ b/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)}" diff --git a/lib/stripe/invoice.rb b/lib/stripe/invoice.rb index 4b2f9623..c865ec2b 100644 --- a/lib/stripe/invoice.rb +++ b/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 diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 382778d4..690417db 100644 --- a/lib/stripe/stripe_client.rb +++ b/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 diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index f43502d5..b7309abb 100644 --- a/lib/stripe/stripe_object.rb +++ b/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 diff --git a/lib/stripe/util.rb b/lib/stripe/util.rb index 2f28c4fb..8acadd35 100644 --- a/lib/stripe/util.rb +++ b/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] diff --git a/lib/stripe/webhook.rb b/lib/stripe/webhook.rb index b1e998b9..0e00890f 100644 --- a/lib/stripe/webhook.rb +++ b/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 diff --git a/stripe.gemspec b/stripe.gemspec index cf374fca..90e7a30e 100644 --- a/stripe.gemspec +++ b/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" diff --git a/test/stripe/api_resource_test.rb b/test/stripe/api_resource_test.rb index de3c518f..336b31c5 100644 --- a/test/stripe/api_resource_test.rb +++ b/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 diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index cac2bd50..93c7649e 100644 --- a/test/stripe/stripe_client_test.rb +++ b/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 diff --git a/test/stripe/stripe_object_test.rb b/test/stripe/stripe_object_test.rb index bb3e917c..a775a028 100644 --- a/test/stripe/stripe_object_test.rb +++ b/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 diff --git a/test/stripe/webhook_test.rb b/test/stripe/webhook_test.rb index ac8292ec..cf499024 100644 --- a/test/stripe/webhook_test.rb +++ b/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 = {}) diff --git a/test/test_helper.rb b/test/test_helper.rb index 74047c0e..41027c86 100644 --- a/test/test_helper.rb +++ b/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