Project

General

Profile

Refactor #5713

Remove method definitions from included block when using Concern

Added by Petr Chalupa over 6 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Target version:
Branch:
Difficulty:
Triaged:
Yes
Bugzilla link:
Pull request:
Fixed in Releases:
Found in Releases:

Description

Hi fellow Rubyists,

I saw methods defined directly in included block when using Concern several times.

module AModule
  extend ActiveSupport::Concern

  included do
   # this is BAD
   def a_method
    end
  end
end

It is a bad practice and it breaks things, all instance methods should go to the AModule, and all class methods to a ClassMethods module.

- It dynamically creates methods in each class, they are not shared through module, it is slow.
- It cannot be accessed for extension, there is no module to include to.
- it breaks `super` calls.

We should avoid using this and go for:

module AModule
  extend ActiveSupport::Concern

  included do
    # e.g.
    has_one :user
  end

  module ClassMethods
    def a_method
    end
  end
end

Thanks.

I'll create issue to remove these bad definitions.

Petr


Related issues

Related to Katello - Bug #6317: rake katello:reindex fails with NoMethodError: undefined method `delete_index' for Katello::PuppetModule:Class Closed2014-06-20
Copied to Foreman - Refactor #5714: Remove method definitions from included block when using ConcernNew2014-05-14

Associated revisions

Revision da7584bd (diff)
Added by Eric Helms over 6 years ago

Fixes #5713: Removes instance method declarations from included block of concerns.

Revision 9e818043
Added by Eric Helms over 6 years ago

Merge pull request #4108 from ehelms/fixes-5713

Fixes #5713: Removes instance method declarations from included block of...

History

#1 Updated by Petr Chalupa over 6 years ago

  • Description updated (diff)

#2 Updated by Petr Chalupa over 6 years ago

  • Tracker changed from Bug to Refactor

#3 Updated by Petr Chalupa over 6 years ago

  • Copied to Refactor #5714: Remove method definitions from included block when using Concern added

#4 Updated by Eric Helms over 6 years ago

  • Target version set to 48
  • Triaged changed from No to Yes

#5 Updated by Eric Helms over 6 years ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

Applied in changeset katello|commit:da7584bd9c6a14d3c641cb8777a58b4a8211cd26.

#6 Updated by David Davis over 6 years ago

  • Related to Bug #6317: rake katello:reindex fails with NoMethodError: undefined method `delete_index' for Katello::PuppetModule:Class added

#7 Updated by Eric Helms over 6 years ago

  • Legacy Backlogs Release (now unused) set to 13

Also available in: Atom PDF