In Twin#create_accessors, don't overwrite existing methods

[niels]

Proposed fix for #36.

[apotonick]

Very glad you're addressing this, @niels! :heart:

For internal reference, here are related issues:

• https://github.com/apotonick/reform/issues/334
• https://github.com/apotonick/reform/issues/312
• https://github.com/apotonick/disposable/issues/36

[niels]

No worries :kissing_heart:

While this PR fixes the issue at hand, I actually wonder if a better solution wouldn't be to reconsider the module inclusion itself (as alluded to in #36). It isn't clear to me what the benefits are of including the accessors through a module. @apotonick Why did you feel that this was superior to simply defining the accessors on the twin itself? I see mainly drawbacks at this point.

First, it leads to subtle "bugs" (perhaps not a "bug" in the traditional sense because everything works as designed, but a "bug" insofar as the behaviour is very surprising and certainly not traditional OOP) as discovered here and in the referenced other issues. It basically "destroys" traditional inheritance.

Second, it "spams" the object graph to such a degree that it is very difficult for outsiders and newcomers to fully understand what is going on. As @ArthurN pointed out in apotonick/reform#312: "there is so much metaprogramming going on here I can't follow it all". (Remember that it generates one discrete module per property, so that without even trying, you can easily end up with dozens of those in your .ancestors – all of them anonymous / unnamed!)

This would all be fine if it brought amazing benefits as well. In this case however, I just can't see any. Even the code looks somewhat awkward with mod = Module.new { define_method }; include mod of course fundamentally being a more verbose version of the functionally (almost) identical one-liner define_method.

Still, I'm sure you had something else in mind when coming up with that implementation which I just don't see right now (I'm new here!), so I'd be interested in a brief rationale for using Module.new here and in Coercion.

[niels]

So my proposed patch breaks something, somewhere in the interplay between disposable, reform, validations, and/or populators. I have been fighting with this all day and am pretty lost at this point.. Any extra set of eyes would be highly appreciated!

With my patch, validations added on top of an inherited form don't work. Without it, they do. The following should result in one error being added to each six of the forms. This is what happens without the patch. With the patch however, the first two forms don't get any errors (as described in-line below):

require "reform/form/active_model/validations"

Reform::Form.class_eval do
  feature Reform::Form::ActiveModel::Validations
end

MainModel = Struct.new(:sub_model)
SubModel  = Struct.new(:a_property)

# For inherited forms, the sub-form's model must be passed into the form
# initialized explicitly for validations from the sub-form's contract to be
# applied.

class NonValidatedForm < Reform::Form
  property :sub_model, populate_if_empty: SubModel do
    property :a_property
  end
end

class ValidatedInheritedForm < NonValidatedForm
  property :sub_model, inherit: true do
    validates :a_property, presence: true
  end
end

# Relying on populate_if_empty for #sub_model to be set

form = ValidatedInheritedForm.new(MainModel.new)
form.validate(sub_model: { a_property: "" })
puts form.errors.count #=> Is 0 but should be 1

# Setting #sub_model manually

form = ValidatedInheritedForm.new(MainModel.new)
form.sub_model = SubModel.new
form.validate(sub_model: { a_property: "" })
puts form.errors.count #=> Is 0 but should be 1

# Explicitly passing #sub_model into the Form initializer

form = ValidatedInheritedForm.new(MainModel.new(SubModel.new))
form.validate(sub_model: { a_property: "" })
puts form.errors.count #=> Is 1 as expected

# For non-subclassed forms however, everything works expected.

class ValidatedSingleClassForm < Reform::Form
  property :sub_model, populate_if_empty: SubModel do
    property :a_property
    validates :a_property, presence: true
  end
end

# Relying on populate_if_empty for #sub_model to be set

form = ValidatedSingleClassForm.new(MainModel.new)
form.validate(sub_model: { a_property: "" })
puts form.errors.count #=> Is 1 as expected

# Explicitly passing #sub_model into the Form initializer

form = ValidatedSingleClassForm.new(MainModel.new(SubModel.new))
form.validate(sub_model: { a_property: "" })
puts form.errors.count #=> Is 1 as expected

# Setting #sub_model manually

form = ValidatedSingleClassForm.new(MainModel.new)
form.sub_model = SubModel.new
form.validate(sub_model: { a_property: "" })
puts form.errors.count #=> Is 1 as expected

[apotonick]

The reason I define the accessors via an anonymous Module is that you can override them using another module. If defined directly on the twin class, you can't do

ArtistForm.new.extend(ModuleOverridingAnAccessor)

This is a Ruby tweak and is not really meta-programming but a thing I do virtually everywhere to allow users to override methods using modules.

[apotonick]

@niels I think I fixed it. May I ask you to point your Gemfile to disposable/master and see if the problem still exists?

Basically, Declarative now tells Disposable that it is inheriting and thus Disposable can omit creating the accessors, again.

[apotonick]

@niels This PR is partly merged, but I simplified it. I suppose the last commit is redundant, as we simply do not create any accessor anymore in a subclass. Please test against disposable/master, and thanks tons for your help! :heart:

[niels]

@apotonick Excellent, that will do it.

I contemplated a similar alternative after you gave out the rationale for using modules but couldn't get to it in time. Moving the inheritance check up to before #create_accessors is called is perfect as it will save us a couple of object allocations and will in addition reduce convolution in the inheritance tree(s). Making it declarative makes sense as well. Thanks for taking over!