module redesign #35

Closed
EppO opened this Issue Jan 24, 2012 · 4 comments

Projects

None yet

2 participants

@EppO
Member
EppO commented Jan 24, 2012

Introduction of new features got really messy in the legacy code (1.x). I started to try to clean it up splitting the ORM adapters code into several files but I think I can do more in the big role.rb file. I should split it up into several modules, 1 module per file.
Targeting this for 3.0 release

@EppO EppO was assigned Jan 24, 2012
@EppO
Member
EppO commented Jan 24, 2012

Would be good to clean up specs too...

@hazah
hazah commented Jan 28, 2012

Hey there, I got a question. In the code I keep noticing this theme:

    def self.included(base)
      base.extend ClassMethods
      base.send :include, InstanceMethods
    end

    module InstanceMethods
      def applied_roles
        self.roles + Rolify.role_cname.where(:resource_type => self.class.to_s, :resource_id => nil)
      end
    end

I was wondering, (I'm not making any suggestions on the matter, it's all up to you) why have the two levels of indirection? First, include being private forced you to use a send in its place. Then two more lines to read, one of which adds very little since, by virtue of being an included module, its methods would be InstanceMethods anyway. I'm not saying you should change anything here, I'm merely wondering about the rationale. Is it cleaner than:

    def self.included(base)
      base.extend ClassMethods
    end

    def applied_roles
      self.roles + Rolify.role_cname.where(:resource_type => self.class.to_s, :resource_id => nil)
    end
@EppO
Member
EppO commented Jan 30, 2012

That's exactly where I am heading to. I tried several ways of how to "link" class methods and instance methods on the user and also resources classes. I just read this blog post and I intend to redesign this library this way, and also to split up the big role.rb files. I've also looked at ActiveSupport::Concern strategy, but I don't know yet if I put ActiveSupport in the loop.

@hazah
hazah commented Jan 31, 2012

It's the same article that brought this to my attention.

@EppO EppO referenced this issue Feb 9, 2012
Closed

specs refactoring #38

@EppO EppO added a commit that referenced this issue Mar 24, 2012
@EppO EppO started module redesign
split up big role.rb into multiple files
not pleased yet about InstanceMethods module
refs #35
8af74c6
@EppO EppO added a commit that closed this issue Mar 26, 2012
@EppO EppO finished to split up role.rb
closes #35

updated railtie to extend ActiveRecord::Base class with Rolify
adding Mongoid support in railtie
closes #26
672a62e
@EppO EppO closed this in 672a62e Mar 26, 2012
@EppO EppO removed their assignment Jul 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment