Is this if still required when the method is always called when record is present?

@p8 I think so because we call this method with reset_inverse_instance(target) hence target might be nil even if record is present.

I suppose that's unclear because we call the argument record, maybe we should call it target instead?

Actually we can even avoid the parameter since we already can retrieve target without passing it...

What about:

def reset_target_inverse_instance
  if target
    inverse_association_for(target)&.reset
  end
end

?

Hmm, I'm not sure 🤔
I think it's clearest if we use the same method as for the elsif target branch, and check for target there.

          if record
            raise_on_type_mismatch!(record)
            remove_inverse_instance(target) if target
            set_inverse_instance(record)
            @updated = true
          elsif target
            remove_inverse_instance(target)

Or maybe we should move the target check to remove_inverse_instance?

      def remove_inverse_instance(record)
        if record && inverse = inverse_association_for(record)
          inverse.inversed_from(nil)
        end
      end

Than we can simplify to the elsif to an else as well:

          if record
            raise_on_type_mismatch!(record)
            remove_inverse_instance(target)
            set_inverse_instance(record)
            @updated = true
          else
            remove_inverse_instance(target)

I can see it would make sense to remove the target argument, but we pass it for a lot of methods:

@p8 The problem of using the same method for both cases is that they don't do exactly the same thing:

• remove_inverse_instance(target) will set the inverse to nil, the inverse is loaded and cached in memory
• reset_inverse_instance(record) resets the inverse association, which will be retrieved via a SQL query the next time is accessed

We could reset the inverse association in both cases but this leads to a little performance degradation because we might trigger a SQL query even when it's not required (the elsif part).

I've just pushed a rename target => record to reset_inverse_instance method in order to make the code clearer.
We can do further refactors later on, if the current changes are acceptable; I hope is ok for you 🙂 .

@p8 pinging you incase the notification was unnoticed 🙇