Skip to content
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

Allows you to check if a field has changed to a particular value #13131

Merged
merged 1 commit into from Dec 31, 2013

Conversation

@gja
Copy link
Contributor

gja commented Dec 2, 2013

I'll add documentation if people are interested in this change

model.name_changed?("Ringo")
model.name_changed?("John" => "Ringo")

@senny
Copy link
Member

senny commented Dec 2, 2013

Related to #12763

@gja
Copy link
Contributor Author

gja commented Dec 2, 2013

@senny Did you link the right ticket? The one you linked to seems like a performance issue

@senny
Copy link
Member

senny commented Dec 2, 2013

@gja I absolutely did not 😓 should be good now.

@anthonybailey
Copy link

anthonybailey commented Dec 2, 2013

I find that the example line of code
model.name_changed?("Ringo")
reads ambiguously.

It could mean that the name changed from "Ringo", or that it changed to "Ringo".

@gja
Copy link
Contributor Author

gja commented Dec 2, 2013

Would you feel better about

model.name_changed?(to: "Ringo")

@chancancode
Copy link
Member

chancancode commented Dec 2, 2013

Personally not a fan of introducing a mini DSL inside a method, particularly _changed?( from => to ). It seems like that didn't work out too well when we tried, like #to_json( a million nested options ).

changed?( from: ..., to: ... ) kind of has the same ring to it, but I guess it's really just a method that takes options that happens to read nicely, so maybe that one is better. 


Sent from Mailbox for iPhone

On Mon, Dec 2, 2013 at 9:32 AM, Tejas Dinkar notifications@github.com
wrote:

Would you feel better about

model.name_changed?(to: "Ringo")

Reply to this email directly or view it on GitHub:
#13131 (comment)

@acapilleri
Copy link
Contributor

acapilleri commented Dec 2, 2013

+1 @chancancode there is a gem that do something like your proposal
https://github.com/yasuoza/activemodel-attribute_changed_specification.
Also changed?( from: ..., to: ... ) is much flexible to introduce future Proc as hash value

@acapilleri
Copy link
Contributor

acapilleri commented Dec 2, 2013

the only thing is that it is a little long-winded as code

@gja
Copy link
Contributor Author

gja commented Dec 2, 2013

@acapilleri Looks like that gem does fundamentally the same thing, but as an alias_method_chain (hmm, maybe I can do that as well).

I'm not sure how to make the code shorter. Maybe by introducing an intermediate private method/class.

@acapilleri
Copy link
Contributor

acapilleri commented Dec 9, 2013

@gja I mean that @model.changed?( from: 'no_changed_Value', to: 'new_Value') seems long but it's only aesthetic concern

cc / @dhh

@dhh
Copy link
Member

dhh commented Dec 9, 2013

What's the use case for @model.changed?(from: 'something')? I can see the to-case.

@gja
Copy link
Contributor Author

gja commented Dec 9, 2013

@dhh I can see the value in specifying 'to', and both 'from and to' together. The latter could be used to check if an object has moved from one state to another, and potentially trigger some action / callbacks.

cancel_stripe_subscription if subscription_changed?(from: 'subscribed', to: 'cancelled')

I suppose it would make sense to implement 'from' just to be complete / so that there are no surprises.

@dhh
Copy link
Member

dhh commented Dec 9, 2013

Gotcha. I suppose that makes sense. I like this format.

On Dec 9, 2013, at 9:42 AM, Tejas Dinkar notifications@github.com wrote:

@dhh I can see the value in specifying to', and bothfrom and to' together. The latter could be used to check if an object has moved from one state to another, and potentially trigger some action / callbacks.

cancel_stripe_subscription if subscription_changed?(from: 'subscribed', to: 'cancelled')

I suppose it would make sense to implement 'from' just to be complete / so that there are no surprises.


Reply to this email directly or view it on GitHub.

@acapilleri
Copy link
Contributor

acapilleri commented Dec 9, 2013

Sorry I mean @model.attrName_changed?( from: 'no_changed_Value', to: 'new_Value')

@acapilleri
Copy link
Contributor

acapilleri commented Dec 9, 2013

cool! @gja ping me when you update your Pr, because I have worked in the gem above. and could be great add also the lambdaexpressions as possible values

@gja
Copy link
Contributor Author

gja commented Dec 10, 2013

@acapilleri I've update the PR, and I've implemented the API so it accepts lambda's for from and to.

I'm guessing you mean something like this:
name_changed?(to: -> (str) { str.starts_with?("foo") })

I've used the case comparator operator (===) to check this equality (=== evaluates the block, and defaults to == for strings and numbers). I'm wondering if this is hacky.

@chancancode
Copy link
Member

chancancode commented Dec 10, 2013

@gja I think using === for implementing proc support is definitely okay, it's a pretty common Ruby idiom, and it would allow things like Regexp etc to work as well.

Whether we should support procs, Regexp etc is a different question. I'm leaning towards no, because like I said I'm not a big fan of these "mini-DSL" things inside a method.

On the other hand, I went through some of my code that might find use for this xxx_changed?(...) feature, and most of them can't actually take advantage of this because I need to check more than simple equality (matching the old value against a regexp is most common)...

@carlosantoniodasilva
carlosantoniodasilva reviewed Dec 10, 2013
View changes
activemodel/test/cases/dirty_test.rb Outdated
assert !@model.name_changed?(from: "Pete")
assert @model.name_changed?(to: -> name { name.start_with? "Ring" })
assert !@model.name_changed?(to: -> name { name.end_with? "Ring" })
end

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2013

Member

Please use assert_not

@carlosantoniodasilva
carlosantoniodasilva reviewed Dec 10, 2013
View changes
activemodel/lib/active_model/dirty.rb Outdated
def attribute_changed?(attr, options = {})
result = changed_attributes.include?(attr)
result &&= options[:to] === changes[attr].last if options.key?(:to)
result &&= options[:from] === changes[attr].first if options.key?(:from)

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2013

Member

You should probably cache the changes[] call.

@acapilleri
Copy link
Contributor

acapilleri commented Dec 11, 2013

Considering this module is most used in active record, where sometimes tracking the changes can generate a bunch of code, could be nice, for example, use a method like:
user.role_changed?(to: -> (role) { ["admin", "super_admin"].incude?(role) }) instead of
user.role_changed?(to: -> "admin" }) or user.role_changed?(to: -> "super_admin" }

@chancancode
Copy link
Member

chancancode commented Dec 11, 2013

Personally, I think using a proc in these scenarios is not very readable:

if user.role_changed?(to: -> (role) { ["admin", "super_admin"].incude?(role) })
  # ...
end

# ...compared to...

if user.role_changed? && (user.role == "admin" || user.role == "super_admin")
  # ...
end

So speaking for myself, I'd probably never use a proc like this, and I'd prefer not having to read code that does that. However, using === does give you things like Regexp and Range matching that could be quite nice in theory. However i couldn't come up with any realistic examples for those. (I checked my code again, and then I realized the Regexp examples I mentioned earlier they can't actually use this, because they need to dereference associations.)

It also could in theory cause problems for certain Ruby types that overrides === to mean something that the user doesn't expect. However, I can't think of any realistic cases where that could cause a problem either.

So, in summary, I personally don't prefer the === unless there are cases that justifies them.

@gja
Copy link
Contributor Author

gja commented Dec 11, 2013

I could personally go either way wrt to accepting lambdas (but lean towards @chancancode's arguments).

I suppose we could try a minimal approach, and start by only accepting values in. If a lot of people use this and want lambdas, then we could include that later.

This is my first major pull request to rails. How do these differences in opinion usually get resolved?

@acapilleri
Copy link
Contributor

acapilleri commented Dec 11, 2013

IMHO

if user.role_changed?(to: -> (role) { ["admin", "super_admin"].incude?(role) }) 

is expressive code than user 3 boolean relations
and much in fromcase

if user.role_changed?(from: -> (role) { ["admin", "super_admin"].incude?(role) })

Also in many cases ["admin", "super_admin"] is a costant or result of a query.
I worked with some application with many roles where something like that would have been useful.

Regarding === maybe the use of calland == could be a solution

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Dec 11, 2013

I don't think we need to support procs/lambdas here, having the basic functionality will solve most of the cases.

@gja
Copy link
Contributor Author

gja commented Dec 12, 2013

@acapilleri @chancancode @carlosantoniodasilva It looks like most people are against lambdas.

@acapilleri Would your usecase be solved by accepting an enumerable, to check inclusion? That actually could read well

user.role_changed?(from: %w(admin super_admin))

@chancancode
Copy link
Member

chancancode commented Dec 12, 2013

@gja 👎 that would make it impossible to make this work with pgsql Array and JSON types, for example. I think == is probably all that is needed, at least for the time being. JustRuby(tm) to the rescue:

if user.role_changed? && ["admin", "super_admin"].incude?(user.role_was)
  # ...
end
@acapilleri
Copy link
Contributor

acapilleri commented Dec 12, 2013

@gja use an array as value could be inconsistent.
The minimal approach without lambda is good, thanks anyway!

@chancancode
chancancode reviewed Dec 14, 2013
View changes
activemodel/lib/active_model/dirty.rb Outdated
def attribute_changed?(attr, options = {})
result = changed_attributes.include?(attr)
result &&= options[:to] == __send__(attr) if options.key?(:to)
result &&= options[:from] == changes[attr].first if options.key?(:from)

This comment has been minimized.

Copy link
@chancancode

chancancode Dec 14, 2013

Member

You probably shouldn't not use changes here, because it makes the code very difficult to follow, and unnecessarily inefficient :(

You see, changes internally calls attribute_change, which calls attribute_changed? again (!)

Seeing that changed_attributes already give you the original value, you should probably use that instead. :)

@chancancode
Copy link
Member

chancancode commented Dec 14, 2013

Hi @gja, when you addressed the comment above, can you also do these? Thanks 💛

  1. Add a CHANGELOG entry
  2. Rebase against master, resolve the conflict and squash your commits. You can probably reuse what you added in the CHANGELOG for this.
model.name_changed?(from: "Pete", to: "Ringo")
@gja
Copy link
Contributor Author

gja commented Dec 31, 2013

@chancancode Did you get a chance to look at the lastest commit?

chancancode added a commit that referenced this pull request Dec 31, 2013
Allows you to check if a field has changed to a particular value
@chancancode chancancode merged commit f3a8be3 into rails:master Dec 31, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@chancancode
Copy link
Member

chancancode commented Dec 31, 2013

@gja thanks 😁

@momer
Copy link
Contributor

momer commented Feb 19, 2014

👍 Thank you, this looks great.

@gja gja deleted the gja:changed-accepts-values branch Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.