From 6fcc9aaabe9580476f829aa4103ca2f014152e89 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Tue, 15 Sep 2026 12:04:14 +0200 Subject: [PATCH 1/3] Add Style/PreferDataDefine cop and convert Struct.new usages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requested in review on PR #2855: prefer Data.define over Struct.new for immutable value objects. The custom cop flags Struct.new sends without autocorrection (mutability and keyword semantics differ, so each hit needs a human decision) and is required from .rubocop.yml. Converts the seven Struct.new test doubles in spec/helpers/email_header_helper_spec.rb to Data.define — the only usages in the repo. Data.define accepts the same positional arguments here. Refs #2878 --- .rubocop.yml | 3 ++ lib/rubocop/cop/style/prefer_data_define.rb | 22 +++++++++ spec/helpers/email_header_helper_spec.rb | 14 +++--- .../cop/style/prefer_data_define_spec.rb | 49 +++++++++++++++++++ 4 files changed, 81 insertions(+), 7 deletions(-) create mode 100644 lib/rubocop/cop/style/prefer_data_define.rb create mode 100644 spec/rubocop/cop/style/prefer_data_define_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 9b2c450d8..954319c6a 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -4,6 +4,9 @@ inherit_mode: merge: - Exclude +require: + - ./lib/rubocop/cop/style/prefer_data_define + # Find out more about the rubocop cops https://github.com/bbatsov/rubocop/blob/master/config/enabled.yml AllCops: diff --git a/lib/rubocop/cop/style/prefer_data_define.rb b/lib/rubocop/cop/style/prefer_data_define.rb new file mode 100644 index 000000000..e282ba33a --- /dev/null +++ b/lib/rubocop/cop/style/prefer_data_define.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # Prefer Data.define over Struct.new for immutable value objects. + # + # Data defines immutable value objects with keyword-aware equality; + # Struct instances are mutable, so the rewrite is not safely mechanical. + class PreferDataDefine < Base + MSG = 'Prefer `Data.define` over `Struct.new` for immutable value objects.' + + def on_send(node) + return unless node.receiver&.const_type? && node.receiver.const_name == 'Struct' + return unless node.method?(:new) + + add_offense(node) + end + end + end + end +end diff --git a/spec/helpers/email_header_helper_spec.rb b/spec/helpers/email_header_helper_spec.rb index 829ec8b76..985b29bb2 100644 --- a/spec/helpers/email_header_helper_spec.rb +++ b/spec/helpers/email_header_helper_spec.rb @@ -6,7 +6,7 @@ end describe '#mail_to_member' do - let(:member) { Struct.new(:id, :email).new(1, 'test@example.com') } + let(:member) { Data.define(:id, :email).new(1, 'test@example.com') } it 'calls mail with correct arguments for valid email' do allow(helper).to receive(:mail).with( @@ -38,37 +38,37 @@ end it 'returns SkippedEmail for nil email' do - member = Struct.new(:id, :email).new(1, nil) + member = Data.define(:id, :email).new(1, nil) result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'returns SkippedEmail for blank email' do - member = Struct.new(:id, :email).new(1, '') + member = Data.define(:id, :email).new(1, '') result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'returns SkippedEmail for invalid email format' do - member = Struct.new(:id, :email).new(1, 'invalid-email') + member = Data.define(:id, :email).new(1, 'invalid-email') result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'returns SkippedEmail for email missing @ symbol' do - member = Struct.new(:id, :email).new(1, 'invalidexample.com') + member = Data.define(:id, :email).new(1, 'invalidexample.com') result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'returns SkippedEmail for email missing TLD' do - member = Struct.new(:id, :email).new(1, 'invalid@example') + member = Data.define(:id, :email).new(1, 'invalid@example') result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'logs the skip' do - member = Struct.new(:id, :email).new(1, 'bad-email') + member = Data.define(:id, :email).new(1, 'bad-email') allow(Rails.logger).to receive(:info).with(/Skipped email to member 1/) helper.mail_to_member(member, 'Test Subject') diff --git a/spec/rubocop/cop/style/prefer_data_define_spec.rb b/spec/rubocop/cop/style/prefer_data_define_spec.rb new file mode 100644 index 000000000..849ee7383 --- /dev/null +++ b/spec/rubocop/cop/style/prefer_data_define_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'rubocop' +require 'rubocop/rspec/expect_offense' +require 'rubocop/rspec/support' +require_relative '../../../../lib/rubocop/cop/style/prefer_data_define' + +RSpec.describe RuboCop::Cop::Style::PreferDataDefine, :config, :ruby40 do + include RuboCop::RSpec::ExpectOffense + + it 'registers an offense for Struct.new' do + expect_offense(<<~RUBY) + Struct.new(:id, :email) + ^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Data.define` over `Struct.new` for immutable value objects. + RUBY + end + + it 'registers an offense for ::Struct.new' do + expect_offense(<<~RUBY) + ::Struct.new(:id, :email) + ^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Data.define` over `Struct.new` for immutable value objects. + RUBY + end + + it 'does not register an offense for Data.define' do + expect_no_offenses(<<~RUBY) + Data.define(:id, :email) + RUBY + end + + it 'does not register an offense for namespaced receivers' do + expect_no_offenses(<<~RUBY) + Other::Struct.new(:id, :email) + RUBY + end + + it 'does not register an offense for other Struct methods' do + expect_no_offenses(<<~RUBY) + Struct.members + RUBY + end + + it 'does not register an offense for OtherClass.new' do + expect_no_offenses(<<~RUBY) + Member.new(:id, :email) + RUBY + end +end From 66fe07b50641d65f3c333bdefe501773e15f0ed4 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Tue, 15 Sep 2026 12:33:45 +0200 Subject: [PATCH 2/3] Ignore lib/rubocop from Zeitwerk autoloading CI eager-loads the app (config.eager_load = ENV['CI'].present? in test.rb), and autoload_lib walked lib/rubocop, requiring the cop before rubocop was loaded: 'uninitialized constant RuboCop::Cop::Style::Base'. RuboCop cops are loaded by RuboCop itself via the require in .rubocop.yml, not by Rails; exclude the directory from autoloading, as the autoload_lib comment invites. --- config/application.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/application.rb b/config/application.rb index 83b7e22bc..0fa173876 100644 --- a/config/application.rb +++ b/config/application.rb @@ -19,7 +19,7 @@ class Application < Rails::Application # Please, add to the `ignore` list any other `lib` subdirectories that do # not contain `.rb` files, or that should not be reloaded or eager loaded. # Common ones are `templates`, `generators`, or `middleware`, for example. - config.autoload_lib(ignore: %w[assets tasks omniauth]) + config.autoload_lib(ignore: %w[assets tasks omniauth rubocop]) # Configuration for the application, engines, and railties goes here. # From 965625f6f069c142af8a18548d8e9db3e9979b99 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Tue, 15 Sep 2026 17:12:29 +0200 Subject: [PATCH 3/3] Use verifying doubles for member test doubles Review feedback on #2879: the spec's member stand-ins should be RSpec doubles rather than Data.define instances. instance_double(Member) verifies against the real class and satisfies RSpec/VerifiedDoubles. The helper only reads member.id and member.email. --- spec/helpers/email_header_helper_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/helpers/email_header_helper_spec.rb b/spec/helpers/email_header_helper_spec.rb index 985b29bb2..1359a4499 100644 --- a/spec/helpers/email_header_helper_spec.rb +++ b/spec/helpers/email_header_helper_spec.rb @@ -6,7 +6,7 @@ end describe '#mail_to_member' do - let(:member) { Data.define(:id, :email).new(1, 'test@example.com') } + let(:member) { instance_double(Member, id: 1, email: 'test@example.com') } it 'calls mail with correct arguments for valid email' do allow(helper).to receive(:mail).with( @@ -38,37 +38,37 @@ end it 'returns SkippedEmail for nil email' do - member = Data.define(:id, :email).new(1, nil) + member = instance_double(Member, id: 1, email: nil) result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'returns SkippedEmail for blank email' do - member = Data.define(:id, :email).new(1, '') + member = instance_double(Member, id: 1, email: '') result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'returns SkippedEmail for invalid email format' do - member = Data.define(:id, :email).new(1, 'invalid-email') + member = instance_double(Member, id: 1, email: 'invalid-email') result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'returns SkippedEmail for email missing @ symbol' do - member = Data.define(:id, :email).new(1, 'invalidexample.com') + member = instance_double(Member, id: 1, email: 'invalidexample.com') result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'returns SkippedEmail for email missing TLD' do - member = Data.define(:id, :email).new(1, 'invalid@example') + member = instance_double(Member, id: 1, email: 'invalid@example') result = helper.mail_to_member(member, 'Test Subject') expect(result).to be_a(EmailHeaderHelper::SkippedEmail) end it 'logs the skip' do - member = Data.define(:id, :email).new(1, 'bad-email') + member = instance_double(Member, id: 1, email: 'bad-email') allow(Rails.logger).to receive(:info).with(/Skipped email to member 1/) helper.mail_to_member(member, 'Test Subject')