Enable Style/RedundantBegin cop to avoid newly adding redundant begin block

[kamipo]

Currently we sometimes find a redundant begin block in code review
(e.g. #33604 (comment)).

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.

[kaspth]

Thanks, I'm 👍

I was actually looking for this rubocop config, but couldn't find it. How did you find it?

[kaspth]

Fantastic! Now we can just put the ensure at the top level. That always bothered me before.

[kaspth]

Could switch to tap while we're here.

[kaspth]

Huh, I would've thought this begin could also be removed and the rescue could be lined up with else.

[kamipo]

This begin is to rescue an exception raised by commit_transaction (commit_transaction is called when a transaction block is succeeded).

rails/activerecord/test/cases/transactions_test.rb

Lines 572 to 585 in 49f9dff

	def test_rollback_when_commit_raises
	assert_called(Topic.connection, :begin_db_transaction) do
	Topic.connection.stub(:commit_db_transaction, -> { raise("OH NOES") }) do
	assert_called(Topic.connection, :rollback_db_transaction) do
	e = assert_raise RuntimeError do
	Topic.transaction do
	Topic.connection.materialize_transactions
	end
	end
	assert_equal "OH NOES", e.message
	end
	end
	end
	end

This begin couldn't be removed since a begin could be ommited only when toplevel in methods and do-end blocks, but here is in if-else block.

[kamipo]

I was actually looking for this rubocop config, but couldn't find it. How did you find it?

.rubocop.yml in our rails apps has no DisabledByDefault: true.

So it is easy to find the cop to police the redundant code.

% cat redundant_begin.rb
def foo(cond)
  yield cond
end

foo do |cond|
  begin
  ensure
    if cond
    else
      begin
      ensure
        nil
      end
    end
  end
end

% be rubocop redundant_begin.rb
Inspecting 1 file
C

Offenses:

redundant_begin.rb:6:3: C: Style/RedundantBegin: Redundant begin block detected.
  begin
  ^^^^^

1 file inspected, 1 offense detected

[kamipo]

Could switch to tap while we're here.

Modified to use tap.

[kamipo]

This begin is to rescue an exception raised by commit_transaction (commit_transaction is called when a transaction block is succeeded).

rails/activerecord/test/cases/transactions_test.rb

Lines 572 to 585 in 49f9dff

	def test_rollback_when_commit_raises
	assert_called(Topic.connection, :begin_db_transaction) do
	Topic.connection.stub(:commit_db_transaction, -> { raise("OH NOES") }) do
	assert_called(Topic.connection, :rollback_db_transaction) do
	e = assert_raise RuntimeError do
	Topic.transaction do
	Topic.connection.materialize_transactions
	end
	end
	assert_equal "OH NOES", e.message
	end
	end
	end
	end

This begin couldn't be removed since a begin could be ommited only when toplevel in methods and do-end blocks, but here is in if-else block.

[kaspth]

Cool, thanks!

[kaspth]

Does rubocop have anything to remove send on the now public define_method and others? Or passing the block as a param instead of using yield like we did for _read_attribute?

[kamipo]

Unfortunately any cop doesn't exist for now.

% cat send_and_block_param.rb
Object.send(:define_method, :foo) {}
Object.send(:remove_method, :foo)

def foo1
  bar { yield }
end

def foo2(&block)
  bar(&block)
end

def bar
  yield
end

% be rubocop send_and_block_param.rb
Inspecting 1 file
.

1 file inspected, no offenses detected