[PATCH] define_read_methods should be a class method

Your example might be invalid. Why would you #alias_method when you could #alias_attribute?

http://caboo.se/doc/classes/Module.html#M003439 for more info.

Agreed. #alias_attribute is the proper technique to use here.

Whilst the given example may have turned out to be invalid, I don't agree that this is reason to ignore this patch which improves the overall quality of the code. Law of demeter violations should be discouraged and this patch removes several violations by moving a method to where it more naturally belongs.

Considering the patch a) improves the quality/design of the code and b) passes all existing tests then why can't this be applied? Its an easy win.

Please see the original article for a more in-depth explanation from the patch author:

http://www.relevancellc.com/2007/7/26/twir-july-26-2007-activerecord

To start with, the main reason given for this patch is not being able to do "alias_method :body, :content", which is not a valid reason.

Moreover, this patch ( as of now ) exposes define_read_methods and define_read_method to both class and object, which changes the current behavior and may not be desirable .

Also, the patch adds a TODO note, which is just delaying work or adding more work for future. Plus in the context of this patch, I don't really see any need of "delegate :define_read_methods, :define_read_method, :to => 'self.class'", the patch should probably remove all instances where it's called as an object method.

In addition, if someone calls "define_read_methods" directly, it overrides behavior defined by "@@generate_read_methods".

Having said that, I'm not really against the basic idea behind the patch. Just that I fail to see any gains by doing so, but just adding scope for unwanted errors.

Thanks.

* Thanks for pointing out alias_attribute! I had never used it. Given Ruby's "There is more than one way to do it" nature, I still think alias_method should be able to work. Why add arbitrary rules? * Exposing define_read_method to the class is *clearly* desirable. Leaving it exposed on object is a compromise, in case there is existing behavior that needs it. I would be happy to remove the object versions if somebody with more familiarity with the code concurs. This would also eliminate the TODO. * I don't see how @@generate_read_methods can be used to aggressively preload methods. It only kicks in via method_missing and id.

What do you think the following snippet do :

ActiveRecord::Base.generate_read_method = false
class Person < ActiveRecord::Base
  define_read_methods
end

I wouldn't say it's an arbitary rule, given the fact that read methods, unlike normal object methods, are defined on fly.

The more of API exposed to public, more rules it adds whenever you want to make any modifications in future. All I'm asking for is *one* *good* *example* where this approach would be useful, or better than whatever is already there.

Well, that's just me. You should definitely try sending an email to rails core list to see what other people have to say.

Thanks.

I see where you are coming from. So let's make the methods private. They still need to be on the class, not the instance.

Agree with that.

Oh but I'm not sure if making them private can prevent people from using'em. Chekc this pastie for an example : http://pastie.caboo.se/82783

Replying to lifofifo:

lifofifo, that example is the incorrect. This is how to make class methods private:

class Foo
  class << self
    private
    def private_method_on_class; end
  end
end
Foo.private_method_on_class
NoMethodError: private method `private_method_on_class' called for Foo:Class

In context of AR/Inheritance :

class Hello
  class << self
    private
    def ruby_doesnt_like_oops
      puts "Hurray"
    end
  end
end

class Hiya < Hello
  ruby_doesnt_like_oops
end

Hiya

$ ruby crap.rb 
Hurray

Private methods cannot be called with any explicit receiver. But for inherited class, you can call it like the example I've shown above. In ruby, unlike other languages, inheritance plays no role in method visibilities.

Hey guys,

I'm the original author of this code. I agree that define_read_methods and the others should be moved to class level.

However, I suspect we will see problems exposing the API to the public:

if someone calls define_read_methods and later in the class definition redefines one of the generated methods manually, the manual definition will replace the generated reader method. so far so good.

But what will happen in development mode? The method reset_column_information will happily get rid of the manually defined version and regenerate the reader method, and the manual definition is gone.

Also, take a look at migration.rb. It recommends calling reset_column_information in migrations. This will also lead to unexpected behavior.

So all in all, this looks a bit hairy to me.

Oh, and BTW, the use case is not convincing to me. Simply write:

  def body; content; end

It's not completely equivalent, but does the job. And I can't imagine that you need aliasing for table columns very often.

Something to make everyone happy : Changeset [7315]

stuthulhu : If you agree, you should probably close the ticket.

Thanks.

I am marking this as fixed, since Changeset [7315] accomplishes my main goal--moving the methods to the class, where they belong.

This changeset breaks the acts_as_modified plugin.

Replying to zdennis:

This changeset breaks the acts_as_modified plugin.

May be you meant [7315] ?

Yes that is what I mean.