Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive for "Remove unused methods" when overriding methods in the superclass #277

Open
hakanai opened this issue Jul 28, 2016 · 1 comment

Comments

@hakanai
Copy link

hakanai commented Jul 28, 2016

We override verified_request? to work around some kind of issue where JSON requests were getting rejected because they didn't have a CSRF token:

class ApplicationController
  # ... omitting other stuff ...
  def verified_request?
    if request.content_type == 'application/json'
      true
    else
      super
    end
  end
end

This method triggers the "Remove unused method" warning even though it's fairly obvious that it is overriding a method in the superclass, which would normally be assumed to be called by the framework.

There are half a dozen other warnings following the same pattern in our codebase.

@optimizasean
Copy link

optimizasean commented Oct 9, 2019

I am having the same issue. When you create custom classes for models (such as for internal elements of arrays), to run .uniq on them, .eql? must be called. If you do not override .eql? and hash, .uniq will never be false(as it will compare not the elements of the class but the class itself) and therefore never remove duplicates.

I.E.
Users model

# == Schema Information
# 
# Table name: users
# 
# id                 :bigint(8)         not null, primary key
# emails          :string             default([]), not null, is an Array
# created_at   :datetime         not null
# updated_at  :datetime         not null
# 

class User < ApplicationRecord
  def emails
    super.map! { |email| email.is_a?(Email) ? email: Email.new(email) }
  end
end

and Email model

# Users email wrapper
class Email
  attr_reader :attributes
  attr_reader :email

  def initialize(email = "")
    @email = email
  end

  def email
    @email
  end

  def email=(email)
    @email = email
  end

  def blank?
    @email.blank?
  end

  def eql?(other)
    @email.to_s == other.email.to_s
  end

  def hash
    @email.to_s.hash
  end

  def to_s
    @email.to_s
  end
end

And in this scenario, best practices catches the eql? (which is called by .uniq) but not hash (which is also called by .uniq). I think disabling it for the entire email.rb file is not a good solution. Inline comments to disable certain checks could handle special cases like this similar to rubocop/reek/...

Inline Disables: #271

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants