refactor ActiveRecord's #become by removing not needed code

[dcrec1]

This refactor throw some warnings when running the tests:

warning: instance variable @changed_attributes not initialized

If this is a problem, I can change the line to be like this:

  became.instance_variable_set("@changed_attributes", @changed_attributes) if defined?(@changed_attributes)

[kaspth]

Sorry, but this is a cosmetic change. Thanks ❤️

[rafaelfranca]

The code you proposed is exactly what it was before 5f6370a, so either the test is wrong, or the bug doesn't exist.

[dcrec1]

@rafaelfranca maybe the bug existed and the test is ok, but some changes in another parts of the code now prevent the bug from happening.

@kaspth actually there is less code, less behaviour and consequently the code is easier to understand and consequently easier to change. I remember some years ago the core team was interested in keeping the code easier to maintain, sorry if this changed, I didn't know.

[rafaelfranca]

Check with the code without warning if the test passes.

[kaspth]

@dcrec1 be nice now 😄

[rafaelfranca]

Less and simpler code is fine, just cosmetic change is not.

[rafaelfranca]

As I suspected the tests are failing now. So that unnecessary code is just to make sure our code has no warnings and I think it is fine. Do you have other way how to write that code so we don't have the warnings and it is simpler?

[kaspth]

This might do:

became.instance_variable_set(:@changed_attributes, defined?(@changed_attributes) ? @changed_attributes : {})

[dcrec1]

I have another way but it actually changes some behaviour. The change would be removing the freeze from this line and changing the committed line to:

became.instance_variable_set("@changed_attributes", changed_attributes)

In case this freeze could be removed, I'll create a new pull request with all the changes in one commit.

[rafaelfranca]

If removing the freeze it will work means that we are mutating an object that should not be mutated, so I'd say it may be a dangerous change.

[dcrec1]

I've tracked this change to this commit. Couldn't really understand why the freeze was added, it's clear to you? Seems the author tried to force some method to be called, but didn't understand why.

[rafaelfranca]

The reason was exactly to force people to not change that object directly and use the right methods because direct changes of it do not reflect the expected behavior.

[dcrec1]

Anyway, after better analyzing and understanding the code, I changed the line to be like:

became.instance_variable_set("@changed_attributes", attributes_changed_by_setter)

This new way doesn't throw warning neither removes the freeze and I think it's better than #changed_attributes because it uses what's already changed instead of re-doing the process.

Is this ok?

[rafaelfranca]

But will not this calculate the changed attributes for the case where @changed_attributes is not defined (the case that trows the exception)?

[dcrec1]

In this case the original #changed_attributes method will be called, he does basically what the code before the refactor did.

[rafaelfranca]

I see. Yeah that would work, but it will be so much more indirection and the code would be hard to follow. Not sure if it is an improvement over the current code.

To avoid one if and one || we are requiring how want to understand this method to look in another file and understand that attributes_changed_by_setter is an alias to attributes_changed and it will not call the ActiveRecord::Dirty version because we called the alias, not the overridden method.

[dcrec1]

But for understanding what this method is doing it shouldn't be necessary to deep into the stack. I think that in this case the variable is well named and explains what it represents and that is very clear that the code is passing the changed attributes to the new class. I agree that the underlying code could be better, maybe changed_attributes shouldn't be overridden and maybe that freeze could be removed, as the original method doesn't use it and the new method, depending of the condition, doesn't use it neither. I'd happy to tried to improve this code if you agree.

[rafaelfranca]

I'll defer this to @sgrif since he wrote the original code.

[rafaelfranca]

BTW, your solution looks good to me, so if he don't have anything against I'll merge it using attributes_changed_by_setter

[dcrec1]

Sorry I sent a wrong commit, will send again.

[rafaelfranca]

Thank. Lets wait @sgrif review