rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
803 stars 277 forks source link

Autocorrect ConsistentParenthesesStyle with omit_parentheses does not handle usages within expressions #1452

Closed DMA57361 closed 1 year ago

DMA57361 commented 1 year ago

Following my report of #1442 I've had reason to run this cop on some of our other projects and have found two more scenarios where autocorrection of FactoryBot/ConsistentParenthesesStyle with omit_parentheses: true will remove parentheses in a way that produces invalid syntax. In fact it might even be the case these shouldn't be offenses as they cannot be corrected in this way?

Here's a contrived example file spec/models/user_spec.rb to demo my points:

# frozen_string_literal: true

require 'rails_helper'

RSpec.describe User, type: :model do
  let(:invoice) {
    invoice = FactoryBot.build :invoice
    invoice.invoice_lines = [
      FactoryBot.build(:invoice_line, description: 'ProductA', value: 1),
      FactoryBot.build(:invoice_line, description: 'ProductB', value: 2),
      FactoryBot.build(:invoice_line, description: 'ProductC', value: 3)
    ]
    invoice.save!
  }

  let(:nested_factory) { FactoryBot.create :user, team: FactoryBot.create(:team, name: 'Support'), active: true }

  specify { expect(invoice.invoice_lines.count).to eq 3 }
end

Run rubocop -a spec/models/user_spec.rb:

$ rubocop -a spec/models/user_spec.rb
Inspecting 1 file
E

Offenses:

spec/models/user_spec.rb:9:18: C: [Corrected] RSpec/FactoryBot/ConsistentParenthesesStyle: Prefer method call without parentheses
      FactoryBot.build(:invoice_line, description: 'ProductA', value: 1),
                 ^^^^^
spec/models/user_spec.rb:9:24: E: Lint/Syntax: unexpected token tSYMBOL
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
      FactoryBot.build :invoice_line, description: 'ProductA', value: 1,
                       ^^^^^^^^^^^^^
spec/models/user_spec.rb:10:18: C: [Corrected] RSpec/FactoryBot/ConsistentParenthesesStyle: Prefer method call without parentheses
      FactoryBot.build(:invoice_line, description: 'ProductB', value: 2),
                 ^^^^^
spec/models/user_spec.rb:11:18: C: [Corrected] RSpec/FactoryBot/ConsistentParenthesesStyle: Prefer method call without parentheses
      FactoryBot.build(:invoice_line, description: 'ProductC', value: 3)
                 ^^^^^
spec/models/user_spec.rb:16:68: C: [Corrected] RSpec/FactoryBot/ConsistentParenthesesStyle: Prefer method call without parentheses
  let(:nested_factory) { FactoryBot.create :user, team: FactoryBot.create(:team, name: 'Support'), active: true }
                                                                   ^^^^^^

1 file inspected, 5 offenses detected, 4 offenses corrected

And the resulting file has a couple of syntax errors (all the invoice_line factories in an array are broken, as is the team factory as a value in a hash):

# frozen_string_literal: true

require 'rails_helper'

RSpec.describe User, type: :model do
  let(:invoice) {
    invoice = FactoryBot.build :invoice
    invoice.invoice_lines = [
      FactoryBot.build :invoice_line, description: 'ProductA', value: 1,
      FactoryBot.build :invoice_line, description: 'ProductB', value: 2,
      FactoryBot.build :invoice_line, description: 'ProductC', value: 3
    ]
    invoice.save!
  }

  let(:nested_factory) { FactoryBot.create :user, team: FactoryBot.create :team, name: 'Support', active: true }

  specify { expect(invoice.invoice_lines.count).to eq 3 }
end

These usages of factorys are not something I actually like - they should probably get a refactor :) - but they're what we've got right now and the rubocop autocorrect shouldn't be mangling them into invalid syntax.

Rubocop version

$ rubocop -V
1.37.1 (using Parser 3.1.2.1, rubocop-ast 1.23.0, running on ruby 2.7.6) [x86_64-darwin18]
  - rubocop-performance 1.15.0
  - rubocop-rails 2.17.0
  - rubocop-rspec 2.14.2
pirj commented 1 year ago

rubocop autocorrect shouldn't be mangling them into invalid syntax

Completely valid. Thanks for reporting!