Closed marten closed 10 years ago
I think as a general policy if we add a method that is defined in an ancestor, we should consider calling super
. It doesn't mean we always do it, but I think it should be considered.
In this case I agree that we should at least call super()
. Like @marten, I'm not sure if we should do arity checks and pass up args based on the arity; the arguments passed into the child method could be completely different from the parent. As a general rule I think it's better to respect the original method interface, since it allows for substitutability, but an #initialize
is probably one of the exceptions to the rule.
I'm planning to extract the constructor part into a separate extension. The contract will be that if you have your own constructor then you don't use virtus' one. This will make things simpler and more obvious. Having said that I don't think I want to complicate the code and make it "smart" wrt initialize and super.
@solnic So do you want to wait for the extraction before merging this? Any way I can help out with that?
@marten yes I want to extract that piece into a separate module and fix super
there. You can of course tackle that if you want :)
I'm gonna call it as fixed because you can now skip including virtus' constructor.
@solnic I take this one as signal to finally implement this in concord also ;)
@solnic And thx. Helps in one of my virtus use cases!
@mbj good to know :) so FYI you can do it like that include Virtus.model { |c| c.constructor = false }
@solnic Good idea!
If Virtus is included in a module, that module might want to add additional initialization, but without a call to super it can't.
It might be I added tests in the wrong places. If that or anything else is wrong, please let me know.
Example use case:
Caveats
I'm not too happy about explicitly expecting that no arguments are expected for
super
. However, if Virtus is included in a simple class with no (explicit) ancestor the ancestor will be Object, whose#initialize
takes no arguments. So without the parentheses aftersuper()
the most basic use case would fail. This does however mean that the user still can't have:I don't know if it's worth muddying up the code with arity checks. If there's an elegant solution, I'd love to hear it.