False positive claiming helper methods are unused if only called from ActionMailer classes

[david-a-wheeler]

False positive claiming helper methods are unused if only called from ActionMailer classes

I believe there is a false positive that claims that helper methods are unused if they are only called from ActionMailer classes.

I think the problem is that RemoveUnusedMethodsInHelpersReview should include a check for method usage in mailers.

Description

The RemoveUnusedMethodsInHelpersReview incorrectly reports helper methods as unused when they are only called from ActionMailer classes, even though this is a fine and common pattern in Rails.

Problem

Helper methods used exclusively in mailers are flagged as unused because the review only analyzes HELPER_FILES, VIEW_FILES, and CONTROLLER_FILES, but excludes MAILER_FILES despite mailers commonly using helper methods. See

File: lib/rails_best_practices/reviews/remove_unused_methods_in_helpers_review.rb
Line: 16

Current code:

interesting_files HELPER_FILES, VIEW_FILES, CONTROLLER_FILES

Example to reproduce

Note: I haven't actually tried this example, this is simplified from a larger program I maintain, but I think it gives the flavor:

Helper file: app/helpers/email_helper.rb

module EmailHelper
  def generate_unsubscribe_url(user)
    # Implementation here
  end
end

Mailer file: app/mailers/notification_mailer.rb

class NotificationMailer < ApplicationMailer
  include EmailHelper
  
  def reminder_email(user)
    @unsubscribe_url = generate_unsubscribe_url(user)
    mail(to: user.email, subject: "Reminder")
  end
end

Result: rails_best_practices incorrectly reports:

remove unused methods (EmailHelper#generate_unsubscribe_url)

Root cause

The gem already defines MAILER_FILES in lib/rails_best_practices/core/check.rb:12:

MAILER_FILES = %r{app/models/.*mailer\.rb$|app/mailers/.*\.rb}.freeze

However, RemoveUnusedMethodsInHelpersReview doesn't use it when determining which files to analyze for method usage.

Proposed fix

File: lib/rails_best_practices/reviews/remove_unused_methods_in_helpers_review.rb
Line: 16

Change from:

interesting_files HELPER_FILES, VIEW_FILES, CONTROLLER_FILES

Change to:

interesting_files HELPER_FILES, VIEW_FILES, CONTROLLER_FILES, MAILER_FILES

Impact

I think this is a backwards-compatible change that reduces false positives, so hopefully there are no downsides.

Current workaround

The obvious workaround to these false positives is to modify the except_methods configuration:

RemoveUnusedMethodsInHelpersCheck: { 
  except_methods: ['EmailHelper#generate_unsubscribe_url'] 
}

That's what I'm currently doing, but I hope there will be a better solution someday :-).