Style/MultilineBlockChain

[dastra-mak]

The following code will raise Style/MultilineBlockChain: Avoid multi-line chains of blocks. :

      expect do
        subject.refresh_lock!(first_record_lock.id, first_record_lock.edit_token)
      end.to raise_error do |error|
        expect(error).to be_a(RecordLock::LockAlreadyReleasedError)
        expect(error.record_lock).to eq(first_record_lock)
        expect(error.record_lock.acquired?).to eq(false)
      end

A solution would be to use

expect { subject.refresh_lock!(first_record_lock.id, first_record_lock.edit_token) }.to raise_error do |error|
  expect(error).to be_a(RecordLock::LockAlreadyReleasedError)
  expect(error.record_lock).to eq(first_record_lock)
  expect(error.record_lock.acquired?).to eq(false)
end

I think this solution is ok when the first block is short, but as soon as it contains multiple lines I'd rather prefer to disable the cop:

      expect do
        subject.refresh_lock!(first_record_lock.id, first_record_lock.edit_token)
        something_else
      end.to raise_error do |error|
        expect(error).to be_a(RecordLock::LockAlreadyReleasedError)
        expect(error.record_lock).to eq(first_record_lock)
        expect(error.record_lock.acquired?).to eq(false)
      end

[foobear]

I think the main reasoning is to avoid unreadable chains in application code like so:

bobs = users.map do |user|
  user.name
end.select do |name|
  name =~ /Bob/
end.join(', ')

In this case, my preferred solution would be

bobs = users
  .map { |user| user.name }
  .select { |name| name =~ /Bob/ }
  .join(', ')

In specs, this occasionally does not match and chaining multi-line blocks is often more readable for complex tests than trying to fix this for the benefit of the cop.

I vote for keeping, but adding 'spec/**/*' to the cop's Exclude list. We might want to take a look at other cops and check if they should be disabled for specs as well.

[denzelem]

I would like to keep this cop. Both examples are hard to read.

bobs = users.map do |user|
  user.name
end.select do |name|
  name =~ /Bob/
end.join(', ')

expect do
  subject.refresh_lock!(first_record_lock.id, first_record_lock.edit_token)
  something_else
end.to raise_error do |error|
  expect(error).to be_a(RecordLock::LockAlreadyReleasedError)
  expect(error.record_lock).to eq(first_record_lock)
  expect(error.record_lock.acquired?).to eq(false)
end

My preferred way would be to say:

def refresh_lock
  subject.refresh_lock!(first_record_lock.id, first_record_lock.edit_token)
  something_else
end

it do
  expect { refresh_lock }.to raise_error do |error|
    expect(error).to be_a(RecordLock::LockAlreadyReleasedError)
    expect(error.record_lock).to eq(first_record_lock)
    expect(error.record_lock.acquired?).to eq(false)
  end
end