Make AM::Dirty less dirty to plugin into AR or other library

[bogdan]

Added new API methods #reset_changes! and #changed_applied!
to control changes state. Previsously you needed to update internal
instance variables, but now API methods is available.

    class Person
      include ActiveModel::Dirty

      define_attribute_methods :name

      # Old way:

      def save
        # do some work
        @previously_changed = changes
        @changed_attributes = nil
      end

      def reload
        # do some work
        @previously_changed = nil
        @changed_attributes = nil
      end

      # New way:

      def save
        # do some work
        changes_applied!
      end

      def reload
        # do some work
        reset_changes!
      end
    end

[egilburg]

1. If calling reset_changes!, should other method be called apply_changes! for consistency?
2. Not sure using ! in method name is a good idea as you are only setting state for later use. I'd conventionally expect a method ending with ! to either possibly raise exceptions, or to perform an immediate destructive/persistive change, neither of which happen here.
3. Risk of shadowing user defined methods (not sure if big deal or avoidable)

[rafaelfranca]

In my opinion these methods should not be public

[rafaelfranca]

@carlosantoniodasilva @josevalim do you have some input about this?

[guilleiguaran]

👍 on this (with all the changes suggested by @rafaelfranca)

[bogdan]

Made all changes suggested by @rafaelfranca

[rafaelfranca]

I think this comment is still wrong. Should not be: Handle <tt>*_changed?</tt> for +method_missing+.?

[rafaelfranca]

I agree with @egilburg about the bang on the method name. I'd only define they without the bang

[bogdan]

Fixed comment, removed bang.

[carlosantoniodasilva]

This should probably be +previous_changes+

[rafaelfranca]

We will need a CHANGELOG entry and a rebase. So I'll merge

[bogdan]

Done

[rafaelfranca]

Merged

[dhh]

We discussing the problem of how "reset_changes" doesn't actually reset the changes, as in restore them to their original value. That's creating a mismatch with reset_, which does do that.

Would be great to see some real use cases that prompted adding reset_changes in the first place. Can people using it on this thread share?

[chancancode]

Ah, this is how we refactored Active Record to use reset_changes in this PR: 9aa1a3d#diff-2bd7c52253c45ccc631ca0d1199d1201R34

It's probably more of a "plugin API" rather than something an ordinary user would use in their app.

[rafaelfranca]

Yes it is intended to be a plugin API.
On Jul 14, 2014 7:54 PM, "Godfrey Chan" notifications@github.com wrote:

Ah, this is how we refactored Active Record to use reset_changes in this
PR: 9aa1a3d#diff-2bd7c52253c45ccc631ca0d1199d1201R34
9aa1a3d#diff-2bd7c52253c45ccc631ca0d1199d1201R34

It's probably more of a "plugin API" rather than something an ordinary
user would use in their app.

—
Reply to this email directly or view it on GitHub
#10816 (comment).

[bogdan]

After a fresh look at this PR reset_changes looks really stupid name now. I agree with @dhh.

The good name would be clear_dirty_data as it is not part of public API outside of activerecord and it don't need to be short and simple.

I think we can still rename it and deprecate old method without causing too many problems to people who are mixing up Dirty outside of ActiveRecord..