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

Inconsistent reporting for FeatureEnvy when using module_function and/or private_class_method #1397

Open
pardaloupa opened this issue Jul 13, 2018 · 4 comments
Labels

Comments

@pardaloupa
Copy link

The following code

module Admins
  class << self
    private
      attr_reader: users
  end

  module_function
  ...
  
  def show_active_users
   ...
   active = active_users
   ...
  end
    
  def users
    @users ||=[]
  end
  
  def active_users
    active =
      users
        .select { |user| user.deleted? == false }
    active.map { |user| user.visible = true
   
    Workspace.fetch_users(active) unless active.empty?
  end
  private_class_method :active_users
end

refers to 'user' more than self (maybe move it to another class?)

Goes away if I use def self.active_users. (prepend self)

The same exact issue occurs when using module_function.

Reek seems to fail to detect that methods defined using module_function/ private_class_method are class instance methods and not instance methods

This is the same issue as #1337 this merge request is also related #1338

@pardaloupa pardaloupa changed the title Inconsistent reporting for FeatureEnvy when using module_function or private_class_method Inconsistent reporting for FeatureEnvy when using module_function and/or private_class_method Jul 13, 2018
@mvz mvz added the defect label Aug 7, 2018
@mvz mvz self-assigned this Aug 7, 2018
@mvz
Copy link
Collaborator

mvz commented Dec 1, 2018

I just started looking into this and the problem is that with module_function both a class method and an instance method are created. So, you can still include the Module and will have active_users as a (private) instance method. Hence, it is a valid candidate to check for FeatureEnvy (and UtilityFunction).

@mvz
Copy link
Collaborator

mvz commented Dec 1, 2018

@troessner wdyt?

@mvz mvz removed their assignment Dec 1, 2018
@troessner
Copy link
Owner

Hmmm, to me it seems like this is one of the edge cases where we'll never have a solution that works for everybody. I agree with @mvz that the check is valid. @mvz what do you think about updating our docs for FeatureEnvy with a proper example and reasoning behind our decision?

@mvz
Copy link
Collaborator

mvz commented Apr 24, 2022

I've just realised that the situation is even more confusing for modules in general: If you define a module for the purpose using it with Class#extend, Reek will treat the methods as instance methods, even though they will be class methods on the extended class.

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

No branches or pull requests

3 participants