On Jul 23, 1:29 pm, David Lee <[email protected]> wrote: > I wanted to propose a way to generate a more accurate changed_columns. > > Currently, Model#[]= sets the changed_columns array whenever the new > value is not equal to the old value. As explained in the > documentation, this approach fails when a new value is unequal before > typecast but equal after typecast, and if a value is set to something > else and then set back to the original value. > > There is also a third way this approach fails: if an object is > modified internally, using a ! method. (This does not go through Model# > []= and the object is still the same object so one can argue that the > change should not be reflected in changed_columns). > > To demonstrate these behaviors: > a.x #=> 1 > a.x = "1" > a.x #=> 1 (same) > a.changed_columns #=> [:x] > > b.y #=> "hi" > b.y = "bye" > b.y = "hi" > b.y #=> "hi" (same) > b.changed_columns #=> [:y] > > c.z #=> "hi" > c.z.gsub "hi", "bye" > c.z #=> "bye" (different) > c.changed_columns #=> [] > > I thought up of a solution that fixes these three scenarios at a > performance cost: > > class Model > def initialize(...) > ... > @saved_values = values > @values = {} > end > > def [](column) > @values[column] || @values[column] = @saved_values[column].dup > end > > def []=(column, value) > @values[column] = typecast_value(column, value) > end > > def changed_columns > @values.keys.find_all {|c| @values[c] != @saved_values[c]} > end > end > > The performance cost comes in Model#[] having to dup and > Model#changed_columns having to detect changes each time. > > Do you see any shortcomings with this approach? Do you think the > accuracy that is attained is worth the performance cost?
You've stated the tradeoffs quite nicely. I think this would simplify some of the internal model code as well. I'm not sure what the performance impact of not caching the output of changed columns is, but it's probably significantly offset by not having to update the changed columns every time columns are changed. I don't think it's going to be a performance bottleneck anyway. Some before and after benchmarks couldn't hurt, though. Long story short, unless it has a high performance cost, I'm in favor of the idea. Do you want to work on a patch? Jeremy --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "sequel-talk" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/sequel-talk?hl=en -~----------~----~----~----~------~----~------~--~---
