Em quinta-feira, 19 de abril de 2012 18h41min03s UTC-3, Jeremy Evans
escreveu:
>
> On Thursday, April 19, 2012 11:50:13 AM UTC-7, Jeremy Evans wrote:
>>
>> On Thursday, April 19, 2012 9:43:49 AM UTC-7, Rodrigo Rosenfeld Rosas
>> wrote:
>>
>>> For example, take a look at this AR model excerpt:
>>>
>>> after_update :remove_options_if_changed_type_was_options
>>> private
>>> def remove_options_if_changed_type_was_options
>>> options.clear if type_changed? && type_was == 'options'
>>> end
>>>
>>> I've even asked for a type_was? method in a Rails pull request at that
>>> time:
>>>
>>> https://github.com/rails/rails/pull/5763
>>>
>>
>> Sequel currently doesn't provide a similar feature, as it doesn't save
>> previous values of columns. It's probably not difficult to add a plugin
>> that would do so, though.
>>
>
> Here's my first attempt, with specs but no documentation yet:
> http://pastie.org/3819298
>
> If anyone could review this diff and provide feedback before I commit it,
> I would appreciate it.
>
I don't know enough yet about Sequel base code, but I didn't understand why
you changed the first part already after starting to read it:
v = typecast_value(column, value)
vals = @values
if new? || !vals.include?(column) || v != (c = vals[column]) ||
v.class != c.class
- changed_columns << column unless changed_columns.include?(column)
- vals[column] = v
+ change_column_value(column, v)
end
end
@@ -1702,6 +1701,13 @@ module Sequel
use_transaction?(opts) ?
db.transaction({:server=>this_server}.merge(opts)){yield} : yield
end
+ # Change the value of the column to given value, recording the
change.
+ def change_column_value(column, value)
+ cc = changed_columns
+ cc << column unless cc.include?(column)
+ @values[column] = value
+ end
For instance, you're no longer calling "typecast_value". Is that
intentional?
Also, particularly I don't see any reasons for changing
changed_columns << column unless changed_columns.include?(column)
to
cc = changed_columns
cc << column unless cc.include?(column)
def column_changes
h = {}
initial_values.each do |column, value|
h[column] = [value, send(column)]
end
h
end
This is just a matter of style, but I'd rather write this as
initial_values.each_with_object do |(column, value), h|
h[column] = [value, send(column)]
end
I've not tested this other style suggestion, but I guess it would work:
def initial_value(column)
initial_values.fetch(column){send(column)}
end
def initial_values
@initial_values ||= {}
end
You wouldn't need the fetch with block if you changed initial_values to
(again, untested):
@initial_values ||= Hash.new{|h, k| send(k) } # or h[k] = send(k), if
that is your intention
Also, you wouldn't need this logic:
value = if initial_values.has_key?(column)
initial_values[column]
else
send(column)
end
Just calling initial_values[column] would suffice.
Also, I'd prefer to read
initial_values[column] = value.clone rescue value
or
initial_values[column] = value.respond_to?(:clone) ? (value.clone rescue
value) : value
instead of
initial_values[column] = if value.respond_to?(:clone)
begin
value.clone
rescue TypeError
value
end
else
value
end
Is there any reason for checking only for TypeError? I'm guessing some
frozen string, maybe. But this is not clear what you're protecting against
and it makes it more difficult to read the code.
The specs and API seem fine to me.
I'm sorry, I know I was not much useful here, but as I said, I don't know
much about Sequel internals to catching some exceptional problematic
situations, so I just gave the pastie a glance :P
I'll appreciate this code integrated on next Sequel release :) Thank you
very much, as always, you're very kind :)
Cheers,
Rodrigo.
--
You received this message because you are subscribed to the Google Groups
"sequel-talk" group.
To view this discussion on the web visit
https://groups.google.com/d/msg/sequel-talk/-/QfOsD4QqHRwJ.
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.
