From 892e38c78e03c11afaa5f01d995e3a21bd92b415 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 21 Dec 2018 02:44:01 +0900 Subject: Enable `Style/RedundantBegin` cop to avoid newly adding redundant begin block Currently we sometimes find a redundant begin block in code review (e.g. https://github.com/rails/rails/pull/33604#discussion_r209784205). I'd like to enable `Style/RedundantBegin` cop to avoid that, since rescue/else/ensure are allowed inside do/end blocks in Ruby 2.5 (https://bugs.ruby-lang.org/issues/12906), so we'd probably meets with that situation than before. --- .../lib/active_support/cache/file_store.rb | 10 +- .../active_support/evented_file_update_checker.rb | 8 +- activesupport/lib/active_support/reloader.rb | 8 +- .../lib/active_support/testing/parallelization.rb | 42 ++++----- .../cache/behaviors/connection_pool_behavior.rb | 56 ++++++----- activesupport/test/core_ext/load_error_test.rb | 7 +- activesupport/test/json/encoding_test.rb | 24 +++-- activesupport/test/share_lock_test.rb | 102 ++++++++++----------- activesupport/test/time_travel_test.rb | 74 +++++++-------- 9 files changed, 153 insertions(+), 178 deletions(-) (limited to 'activesupport') diff --git a/activesupport/lib/active_support/cache/file_store.rb b/activesupport/lib/active_support/cache/file_store.rb index 53a2b07536..de1fb1886c 100644 --- a/activesupport/lib/active_support/cache/file_store.rb +++ b/activesupport/lib/active_support/cache/file_store.rb @@ -108,12 +108,10 @@ module ActiveSupport def lock_file(file_name, &block) if File.exist?(file_name) File.open(file_name, "r+") do |f| - begin - f.flock File::LOCK_EX - yield - ensure - f.flock File::LOCK_UN - end + f.flock File::LOCK_EX + yield + ensure + f.flock File::LOCK_UN end else yield diff --git a/activesupport/lib/active_support/evented_file_update_checker.rb b/activesupport/lib/active_support/evented_file_update_checker.rb index 97e982eb05..b2ae3dd274 100644 --- a/activesupport/lib/active_support/evented_file_update_checker.rb +++ b/activesupport/lib/active_support/evented_file_update_checker.rb @@ -57,11 +57,9 @@ module ActiveSupport # usage of attr_* macros for private attributes, but adds a lot of noise # to our test suite. Thus, we lazy load it and disable warnings locally. silence_warnings do - begin - require "listen" - rescue LoadError => e - raise LoadError, "Could not load the 'listen' gem. Add `gem 'listen'` to the development group of your Gemfile", e.backtrace - end + require "listen" + rescue LoadError => e + raise LoadError, "Could not load the 'listen' gem. Add `gem 'listen'` to the development group of your Gemfile", e.backtrace end end boot! diff --git a/activesupport/lib/active_support/reloader.rb b/activesupport/lib/active_support/reloader.rb index fea18e9712..2f81cd4f80 100644 --- a/activesupport/lib/active_support/reloader.rb +++ b/activesupport/lib/active_support/reloader.rb @@ -50,11 +50,9 @@ module ActiveSupport def self.reload! executor.wrap do new.tap do |instance| - begin - instance.run! - ensure - instance.complete! - end + instance.run! + ensure + instance.complete! end end prepare! diff --git a/activesupport/lib/active_support/testing/parallelization.rb b/activesupport/lib/active_support/testing/parallelization.rb index c5d3a88131..63440069b1 100644 --- a/activesupport/lib/active_support/testing/parallelization.rb +++ b/activesupport/lib/active_support/testing/parallelization.rb @@ -69,31 +69,29 @@ module ActiveSupport def start @pool = @queue_size.times.map do |worker| fork do - begin - DRb.stop_service - - after_fork(worker) - - queue = DRbObject.new_with_uri(@url) - - while job = queue.pop - klass = job[0] - method = job[1] - reporter = job[2] - result = Minitest.run_one_method(klass, method) - - begin - queue.record(reporter, result) - rescue DRb::DRbConnError - result.failures.each do |failure| - failure.exception = DRb::DRbRemoteError.new(failure.exception) - end - queue.record(reporter, result) + DRb.stop_service + + after_fork(worker) + + queue = DRbObject.new_with_uri(@url) + + while job = queue.pop + klass = job[0] + method = job[1] + reporter = job[2] + result = Minitest.run_one_method(klass, method) + + begin + queue.record(reporter, result) + rescue DRb::DRbConnError + result.failures.each do |failure| + failure.exception = DRb::DRbRemoteError.new(failure.exception) end + queue.record(reporter, result) end - ensure - run_cleanup(worker) end + ensure + run_cleanup(worker) end end end diff --git a/activesupport/test/cache/behaviors/connection_pool_behavior.rb b/activesupport/test/cache/behaviors/connection_pool_behavior.rb index 4d1901a173..5e66e0b202 100644 --- a/activesupport/test/cache/behaviors/connection_pool_behavior.rb +++ b/activesupport/test/cache/behaviors/connection_pool_behavior.rb @@ -7,24 +7,22 @@ module ConnectionPoolBehavior threads = [] emulating_latency do - begin - cache = ActiveSupport::Cache.lookup_store(store, { pool_size: 2, pool_timeout: 1 }.merge(store_options)) - cache.clear - - assert_raises Timeout::Error do - # One of the three threads will fail in 1 second because our pool size - # is only two. - 3.times do - threads << Thread.new do - cache.read("latency") - end + cache = ActiveSupport::Cache.lookup_store(store, { pool_size: 2, pool_timeout: 1 }.merge(store_options)) + cache.clear + + assert_raises Timeout::Error do + # One of the three threads will fail in 1 second because our pool size + # is only two. + 3.times do + threads << Thread.new do + cache.read("latency") end - - threads.each(&:join) end - ensure - threads.each(&:kill) + + threads.each(&:join) end + ensure + threads.each(&:kill) end ensure Thread.report_on_exception = original_report_on_exception @@ -34,24 +32,22 @@ module ConnectionPoolBehavior threads = [] emulating_latency do - begin - cache = ActiveSupport::Cache.lookup_store(store, store_options) - cache.clear - - assert_nothing_raised do - # Default connection pool size is 5, assuming 10 will make sure that - # the connection pool isn't used at all. - 10.times do - threads << Thread.new do - cache.read("latency") - end + cache = ActiveSupport::Cache.lookup_store(store, store_options) + cache.clear + + assert_nothing_raised do + # Default connection pool size is 5, assuming 10 will make sure that + # the connection pool isn't used at all. + 10.times do + threads << Thread.new do + cache.read("latency") end - - threads.each(&:join) end - ensure - threads.each(&:kill) + + threads.each(&:join) end + ensure + threads.each(&:kill) end end diff --git a/activesupport/test/core_ext/load_error_test.rb b/activesupport/test/core_ext/load_error_test.rb index 126aa51cb4..8afff4fb9c 100644 --- a/activesupport/test/core_ext/load_error_test.rb +++ b/activesupport/test/core_ext/load_error_test.rb @@ -13,10 +13,9 @@ class TestLoadError < ActiveSupport::TestCase end def test_path - begin load "nor/this/one.rb" - rescue LoadError => e - assert_equal "nor/this/one.rb", e.path - end + load "nor/this/one.rb" + rescue LoadError => e + assert_equal "nor/this/one.rb", e.path end def test_is_missing_with_nil_path diff --git a/activesupport/test/json/encoding_test.rb b/activesupport/test/json/encoding_test.rb index 8062873386..7589ffd0ea 100644 --- a/activesupport/test/json/encoding_test.rb +++ b/activesupport/test/json/encoding_test.rb @@ -21,20 +21,18 @@ class TestJSONEncoding < ActiveSupport::TestCase JSONTest::EncodingTestCases.constants.each do |class_tests| define_method("test_#{class_tests[0..-6].underscore}") do - begin - prev = ActiveSupport.use_standard_json_time_format - - standard_class_tests = /Standard/.match?(class_tests) - - ActiveSupport.escape_html_entities_in_json = !standard_class_tests - ActiveSupport.use_standard_json_time_format = standard_class_tests - JSONTest::EncodingTestCases.const_get(class_tests).each do |pair| - assert_equal pair.last, sorted_json(ActiveSupport::JSON.encode(pair.first)) - end - ensure - ActiveSupport.escape_html_entities_in_json = false - ActiveSupport.use_standard_json_time_format = prev + prev = ActiveSupport.use_standard_json_time_format + + standard_class_tests = /Standard/.match?(class_tests) + + ActiveSupport.escape_html_entities_in_json = !standard_class_tests + ActiveSupport.use_standard_json_time_format = standard_class_tests + JSONTest::EncodingTestCases.const_get(class_tests).each do |pair| + assert_equal pair.last, sorted_json(ActiveSupport::JSON.encode(pair.first)) end + ensure + ActiveSupport.escape_html_entities_in_json = false + ActiveSupport.use_standard_json_time_format = prev end end diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 34479020e1..30a1ddad3f 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -115,68 +115,66 @@ class ShareLockTest < ActiveSupport::TestCase def test_exclusive_conflicting_purpose [true, false].each do |use_upgrading| with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| - begin - together = Concurrent::CyclicBarrier.new(2) - conflicting_exclusive_threads = [ - Thread.new do - @lock.send(use_upgrading ? :sharing : :tap) do - together.wait - @lock.exclusive(purpose: :red, compatible: [:green, :purple]) { } - end - end, - Thread.new do - @lock.send(use_upgrading ? :sharing : :tap) do - together.wait - @lock.exclusive(purpose: :blue, compatible: [:green]) { } - end + together = Concurrent::CyclicBarrier.new(2) + conflicting_exclusive_threads = [ + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + together.wait + @lock.exclusive(purpose: :red, compatible: [:green, :purple]) { } + end + end, + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + together.wait + @lock.exclusive(purpose: :blue, compatible: [:green]) { } end - ] - - assert_threads_stuck conflicting_exclusive_threads # wait for threads to get into their respective `exclusive {}` blocks - - # This thread will be stuck as long as any other thread is in - # a sharing block. While it's blocked, it holds no lock, so it - # doesn't interfere with any other attempts. - no_purpose_thread = Thread.new do - @lock.exclusive { } end - assert_threads_stuck no_purpose_thread + ] - # This thread is compatible with both of the "primary" - # attempts above. It's initially stuck on the outer share - # lock, but as soon as that's released, it can run -- - # regardless of whether those threads hold share locks. - compatible_thread = Thread.new do - @lock.exclusive(purpose: :green, compatible: []) { } - end - assert_threads_stuck compatible_thread + assert_threads_stuck conflicting_exclusive_threads # wait for threads to get into their respective `exclusive {}` blocks - assert_threads_stuck conflicting_exclusive_threads + # This thread will be stuck as long as any other thread is in + # a sharing block. While it's blocked, it holds no lock, so it + # doesn't interfere with any other attempts. + no_purpose_thread = Thread.new do + @lock.exclusive { } + end + assert_threads_stuck no_purpose_thread + + # This thread is compatible with both of the "primary" + # attempts above. It's initially stuck on the outer share + # lock, but as soon as that's released, it can run -- + # regardless of whether those threads hold share locks. + compatible_thread = Thread.new do + @lock.exclusive(purpose: :green, compatible: []) { } + end + assert_threads_stuck compatible_thread - sharing_thread_release_latch.count_down + assert_threads_stuck conflicting_exclusive_threads - assert_threads_not_stuck compatible_thread # compatible thread is now able to squeak through + sharing_thread_release_latch.count_down - if use_upgrading - # The "primary" threads both each hold a share lock, and are - # mutually incompatible; they're still stuck. - assert_threads_stuck conflicting_exclusive_threads + assert_threads_not_stuck compatible_thread # compatible thread is now able to squeak through - # The thread without a specified purpose is also stuck; it's - # not compatible with anything. - assert_threads_stuck no_purpose_thread - else - # As the primaries didn't hold a share lock, as soon as the - # outer one was released, all the exclusive locks are free - # to be acquired in turn. + if use_upgrading + # The "primary" threads both each hold a share lock, and are + # mutually incompatible; they're still stuck. + assert_threads_stuck conflicting_exclusive_threads - assert_threads_not_stuck conflicting_exclusive_threads - assert_threads_not_stuck no_purpose_thread - end - ensure - conflicting_exclusive_threads.each(&:kill) - no_purpose_thread.kill + # The thread without a specified purpose is also stuck; it's + # not compatible with anything. + assert_threads_stuck no_purpose_thread + else + # As the primaries didn't hold a share lock, as soon as the + # outer one was released, all the exclusive locks are free + # to be acquired in turn. + + assert_threads_not_stuck conflicting_exclusive_threads + assert_threads_not_stuck no_purpose_thread end + ensure + conflicting_exclusive_threads.each(&:kill) + no_purpose_thread.kill end end end diff --git a/activesupport/test/time_travel_test.rb b/activesupport/test/time_travel_test.rb index 8c47f2cdc7..4dec0dc334 100644 --- a/activesupport/test/time_travel_test.rb +++ b/activesupport/test/time_travel_test.rb @@ -11,16 +11,14 @@ class TimeTravelTest < ActiveSupport::TestCase def test_time_helper_travel Time.stub(:now, Time.now) do - begin - expected_time = Time.now + 1.day - travel 1.day + expected_time = Time.now + 1.day + travel 1.day - assert_equal expected_time.to_s(:db), Time.now.to_s(:db) - assert_equal expected_time.to_date, Date.today - assert_equal expected_time.to_datetime.to_s(:db), DateTime.now.to_s(:db) - ensure - travel_back - end + assert_equal expected_time.to_s(:db), Time.now.to_s(:db) + assert_equal expected_time.to_date, Date.today + assert_equal expected_time.to_datetime.to_s(:db), DateTime.now.to_s(:db) + ensure + travel_back end end @@ -42,16 +40,14 @@ class TimeTravelTest < ActiveSupport::TestCase def test_time_helper_travel_to Time.stub(:now, Time.now) do - begin - expected_time = Time.new(2004, 11, 24, 01, 04, 44) - travel_to expected_time + expected_time = Time.new(2004, 11, 24, 01, 04, 44) + travel_to expected_time - assert_equal expected_time, Time.now - assert_equal Date.new(2004, 11, 24), Date.today - assert_equal expected_time.to_datetime, DateTime.now - ensure - travel_back - end + assert_equal expected_time, Time.now + assert_equal Date.new(2004, 11, 24), Date.today + assert_equal expected_time.to_datetime, DateTime.now + ensure + travel_back end end @@ -73,21 +69,19 @@ class TimeTravelTest < ActiveSupport::TestCase def test_time_helper_travel_back Time.stub(:now, Time.now) do - begin - expected_time = Time.new(2004, 11, 24, 01, 04, 44) + expected_time = Time.new(2004, 11, 24, 01, 04, 44) - travel_to expected_time - assert_equal expected_time, Time.now - assert_equal Date.new(2004, 11, 24), Date.today - assert_equal expected_time.to_datetime, DateTime.now - travel_back + travel_to expected_time + assert_equal expected_time, Time.now + assert_equal Date.new(2004, 11, 24), Date.today + assert_equal expected_time.to_datetime, DateTime.now + travel_back - assert_not_equal expected_time, Time.now - assert_not_equal Date.new(2004, 11, 24), Date.today - assert_not_equal expected_time.to_datetime, DateTime.now - ensure - travel_back - end + assert_not_equal expected_time, Time.now + assert_not_equal Date.new(2004, 11, 24), Date.today + assert_not_equal expected_time.to_datetime, DateTime.now + ensure + travel_back end end @@ -122,20 +116,18 @@ class TimeTravelTest < ActiveSupport::TestCase def test_time_helper_travel_to_with_subsequent_calls Time.stub(:now, Time.now) do - begin - initial_expected_time = Time.new(2004, 11, 24, 01, 04, 44) - subsequent_expected_time = Time.new(2004, 10, 24, 01, 04, 44) - assert_nothing_raised do - travel_to initial_expected_time - travel_to subsequent_expected_time + initial_expected_time = Time.new(2004, 11, 24, 01, 04, 44) + subsequent_expected_time = Time.new(2004, 10, 24, 01, 04, 44) + assert_nothing_raised do + travel_to initial_expected_time + travel_to subsequent_expected_time - assert_equal subsequent_expected_time, Time.now + assert_equal subsequent_expected_time, Time.now - travel_back - end - ensure travel_back end + ensure + travel_back end end -- cgit v1.2.3