Some refactor for association.

[kennyj]

I'm tackling #5663.
And I think some coding problems are existed on associations (redundant / class hierarchy of methods...).
This PR fix some problems.
Please review it :)

[kevmoo]

@kennyj: I noticed you redefined reset to take in an optional target, but I don't see where this method is used with a non-nil paramater

[kennyj]

@kevmoo kennyj@37cc758#L1R74
I wanted to protect the DRY principle and standardize the reset process :)

[kevmoo]

@kennyj DRY'ing and standardizing are great, but where is this method called with a non-nil param?

The other code path that sets @target is target= which also calls loaded!

I'm trying to understand why we'd want a reset code path that sets @target to anything but nil

[carlosantoniodasilva]

@kevmoo The super [] call on collection association resets the target to an empty array.

@kennyj Do you think other reset overriding should stick with the same method signature?

[kennyj]

@carlosantoniodasilva I think we should use the same method signature possibly (especially public api).

I have another 2 plan.

plan1: we add default_target method.

def reset
  @loaded = false
  @target = default_target
  @stale_state = nil
end

private
  def default_target # override on derived class!
    nil
  end

plan2: without considering the inheritance, we implement reset method separately.

What do you think ?

[kevmoo]

@carlosantoniodasilva : I'm w/ you on the call to super it association_collection. I'm trying to understand the new target param on reset in assocition.rb

[carlosantoniodasilva]

@kenny plan 1 with default_target seems good to me. Another possibility I can think about, in collection association just do:

def reset
  super
  @target = []
end

@kevmoo the new param is required so you can achieve the same result as my example above with super [], which is basically calling reset [] on the parent.

[kennyj]

@carlosantoniodasilva sounds good :-) more simply. I'll update commits. Thank you for reviewing

[kennyj]

done :-)

[kevmoo]

Love it. Is there an associated test case?

[kennyj]

cetrainly, we needed to add some testcases.
... and done :-)

[jonleighton]

This looks fine to me, but I'm not keen on the test. Does this fix any bug? If not, let's just not bother with the test; it's just refactoring.

[kennyj]

@jonleighton Thank you for reviewing.
I agree. I didn't fix any bug.
I rebased and updated this PR.

[jonleighton]

Ok, please squash the commits :) then I'll merge.

[kennyj]

@jonleighton
Done !
Please merge it :-)