Add Attribute#changed? with to: and from: options

[wout]

Adds:

• #changed? with optional from: and to: arguments
• #changes
• #original_value

[wout]

I kept focus: true on the Attribute specs and the two specs with issues. Here is a proposal with passing specs:

https://github.com/wout/avram/tree/attribute-changed-with-explicit-nil/

But this might not be the ideal solution as it treats setting a nil value as change, even if the original value is nil too.

[paulcsmith]

I haven’t been at my computer all day so haven’t done a deep dive but I don’t think we need to match the old behavior exactly. I think if the original value is nil and you set the value to nil again that changed? Should be false. Does that answer the question?

If not lmk and I’ll try to dive in deeper later :D

Thank you so much for this. This is SUPER exciting

[wout]

I think if the original value is nil and you set the value to nil again that changed? Should be false. Does that answer the question?

With the original behaviour, setting any value would make changed? return true, regardless of its contents. That's why I created the second PR (attribute-changed-with-explicit-nil branch) where setting nil, when the original value was nil, results in changed? returning true, because the two mentioned specs (spec/polymorphic_spec.cr:112 and spec/json_column_spec.cr:20) would otherwise fail.

But it does not feel right to do it that way, just to make specs pass.Unless you disagree, of course. 😄

[paulcsmith]

I didn’t notice the other PR! Oops. Yes I agree the old behavior did not make sense. Better to only return true if the value actually changed and change the specs that broke due to the new behavior.

[wout]

Ok, so now everything passes, except for one spec, which I temporarily set to pending. That is a situation where a jsonb column has a default value and a record is created with a explicit nil value. Because, as we discussed before, setting nil on an attribute that was initialized with a nil value does not result in changed? being true, this test fails.

So for this to pass, we should either:

• load the default value from the database and use it as an initial value in an Avram::Attribute if the record has not yet been persisted (if at all possible in a performant way)
• remove this test (as the whole point of a default value on a column is avoiding empty/nil values)

[paulcsmith]

I think you could fix the spec by setting the box to a json value. Then updating it to nil. Lmk if that fixes it! This is lookin hood

[wout]

Here you go. Now all specs run green. :)

[paulcsmith]

Looks great. Thanks for adding this. This will make things like auditing a lot easier!