New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Active Model Dirty API. #16180

Merged
merged 2 commits into from Jul 15, 2014

Conversation

Projects
None yet
5 participants
@rafaelfranca
Member

rafaelfranca commented Jul 15, 2014

I did some changes on this API:

  • reset_changes becomes clear_changes_information. I believe this new name matches with what it is doing.
  • reset_* is deprecated in favor of restore_*. This will avoid confusion with the deprecated reset_changes.
  • undo_changes is now restore_attributes. To match the behaviour of restore_*.

@dhh @chancancode

Deprecate ActiveModel::Dirty#reset_changes in favor of #clear_changes…
…_information

This method name is causing confusion with the `reset_#{attribute}`
methods. While `reset_name` set the value of the name attribute for the
previous value the `reset_changes` only discard the changes and previous
changes.
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 15, 2014

Member

restore_changes feels weird to me, it almost sounds like it's an undo method for the discarding done by clear_changes_information

Member

sgrif commented Jul 15, 2014

restore_changes feels weird to me, it almost sounds like it's an undo method for the discarding done by clear_changes_information

@@ -215,6 +224,16 @@ def attribute_will_change!(attr)
# Handle <tt>reset_*!</tt> for +method_missing+.
def reset_attribute!(attr)

This comment has been minimized.

@chancancode

chancancode Jul 15, 2014

Member

def restore_attribute!(attr)? 😄

@chancancode

chancancode Jul 15, 2014

Member

def restore_attribute!(attr)? 😄

This comment has been minimized.

@chancancode

chancancode Jul 15, 2014

Member

nvm 🙈

@chancancode

chancancode Jul 15, 2014

Member

nvm 🙈

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 15, 2014

Member

restore_values then?

Member

rafaelfranca commented Jul 15, 2014

restore_values then?

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jul 15, 2014

Member

❤️ 💚 💙 💛 💜

Member

chancancode commented Jul 15, 2014

❤️ 💚 💙 💛 💜

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 15, 2014

Member

restore_changed_values to be explicit, maybe?

Member

sgrif commented Jul 15, 2014

restore_changed_values to be explicit, maybe?

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jul 15, 2014

Member

How about #restore_attributes? It just occurred to me that when using this method, you are not really thinking about the "changes", but thinking that you'd like to restore your attributes to their previous values (just like when you use restore_name).

As a bonus, it could take a list of attributes for when you want to restrict it to just a subset of possibly-changed-or-not attributes (e.g. def restore_price; restore_attributes(:currency, :price_in_cents); end).

Not sure if "attributes" is what you call them in the Active Model layer though.

Member

chancancode commented Jul 15, 2014

How about #restore_attributes? It just occurred to me that when using this method, you are not really thinking about the "changes", but thinking that you'd like to restore your attributes to their previous values (just like when you use restore_name).

As a bonus, it could take a list of attributes for when you want to restrict it to just a subset of possibly-changed-or-not attributes (e.g. def restore_price; restore_attributes(:currency, :price_in_cents); end).

Not sure if "attributes" is what you call them in the Active Model layer though.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jul 15, 2014

Member

(or restore_changed_attributes, if it's somehow confusing. we already have a changed_attributes so it pairs nicely)

Member

chancancode commented Jul 15, 2014

(or restore_changed_attributes, if it's somehow confusing. we already have a changed_attributes so it pairs nicely)

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 15, 2014

Member

I like restore_attributes and restore_age. Great symmetry and we bind the term "restore" to specifically deal with the dirty state.

Member

dhh commented Jul 15, 2014

I like restore_attributes and restore_age. Great symmetry and we bind the term "restore" to specifically deal with the dirty state.

Deprecate `reset_#{attribute}` in favor of `restore_#{attribute}`.
These methods may cause confusion with the `reset_changes` that
behaves differently
of them.

Also rename undo_changes to restore_changes to match this new set of
methods.
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 15, 2014

Member

Changed to restore_attributes. Anything else?

Member

rafaelfranca commented Jul 15, 2014

Changed to restore_attributes. Anything else?

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jul 15, 2014

Member

❤️ :shipit:! I would like to explore the usefulness of expanding restore_attributes to take an arg list of attributes, but we can discuss more and do that separately later 😄

Member

chancancode commented Jul 15, 2014

❤️ :shipit:! I would like to explore the usefulness of expanding restore_attributes to take an arg list of attributes, but we can discuss more and do that separately later 😄

rafaelfranca added a commit that referenced this pull request Jul 15, 2014

@rafaelfranca rafaelfranca merged commit 93e09f5 into rails:master Jul 15, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@rafaelfranca rafaelfranca deleted the rafaelfranca:rm-dirty branch Jul 15, 2014

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Jul 19, 2014

Contributor

good job

Contributor

bogdan commented Jul 19, 2014

good job

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment