Declare virtual attribute after 5.1

[casperisfine]

Fix: #59

Inspired from https://github.com/attr-encrypted/attr_encrypted/pull/263 which had similar deprecations. However it's making some tests fail and I'm still unsure why :/

cc @rafaelfranca

[coveralls]

Coverage increased (+0.006%) to 94.893% when pulling 2fcce018a467645a0dbc1f72248b30eb04ee9bdf on fix-5-1-warnings into 154b5a8f37f77e80ac00a8f9591a1eb8c81f9e40 on master.

[rafaelfranca]

I was discussing this deprecation with @sgrif today and asked him to take a look to see the best way to solve this problem. Using the attribute method looks sane, but last time I tried I could not figure it out how to fix the tests.

[sgrif]

I want to better understand why this difference in behavior from ActiveRecord::Store is needed. AR's just marks the store itself as changed (which can probably be removed). I'm curious if trying to mark the individual store attributes as changed was intentional or just a misreading of the AR code. I'm not sure that just calling attribute will be the right thing to do here.

[casperisfine]

I'm curious if trying to mark the individual store attributes as changed was intentional

It is. There is tests for this: https://github.com/byroot/activerecord-typedstore/blob/154b5a8f37f77e80ac00a8f9591a1eb8c81f9e40/spec/active_record/typed_store_spec.rb#L48-L121

[sgrif]

I want to better understand why this difference in behavior from ActiveRecord::Store is needed

[casperisfine]

Because the core idea behind this library is to have as little difference between those "virtual attributes" and attributes backed by their own DB column (short of querying obviously).

ActiveRecord::Store does very little in that regard. No dirty tracking, no coercion, no attribute?, etc.

[sgrif]

I looked into when this was added. It appears my original assessment is correct, and this was a misreading of the old code. The will_change! line was added in https://github.com/byroot/activerecord-typedstore/commit/8d09313ee8caccd081431c3909ce1396ef02687a. The corresponding line which was removed was calling it on the store itself, not the sub-field (same as within Active Record). Since 4.2 detects mutations, the will_change! line can simply be removed.

[casperisfine]

Since 4.2 detects mutations, the will_change! line can simply be removed.

Good to know, that will simplify code in the future. But for now the issue we're having is with 5.1

[sgrif]

5.1 is 3 releases after 4.2. Mutation detection is the only reason that the incorrect will_change! line didn't manifest as a bug after https://github.com/byroot/activerecord-typedstore/commit/8d09313ee8caccd081431c3909ce1396ef02687a. Assuming this library doesn't need to support 4.1, the line can be removed.

[casperisfine]

🤦‍♂️ I read 5.2 because I tested your suggestion and it was causing failures to the test suite.

[coveralls]

Coverage decreased (-0.03%) to 94.856% when pulling f9b2b154fbb9784d108f3de22ec9808c4623eaeb on fix-5-1-warnings into 154b5a8f37f77e80ac00a8f9591a1eb8c81f9e40 on master.

[coveralls]

Coverage decreased (-0.03%) to 94.856% when pulling f9b2b154fbb9784d108f3de22ec9808c4623eaeb on fix-5-1-warnings into 154b5a8f37f77e80ac00a8f9591a1eb8c81f9e40 on master.

[coveralls]

Coverage decreased (-0.03%) to 94.856% when pulling f9b2b154fbb9784d108f3de22ec9808c4623eaeb on fix-5-1-warnings into 154b5a8f37f77e80ac00a8f9591a1eb8c81f9e40 on master.

[coveralls]

Coverage decreased (-0.03%) to 94.856% when pulling f9b2b154fbb9784d108f3de22ec9808c4623eaeb on fix-5-1-warnings into 154b5a8f37f77e80ac00a8f9591a1eb8c81f9e40 on master.

[coveralls]

Coverage decreased (-0.04%) to 94.85% when pulling 98ac6de1b2155a8b5adbf7c2123b098a0cc1e733 on fix-5-1-warnings into 154b5a8f37f77e80ac00a8f9591a1eb8c81f9e40 on master.

[casperisfine]

So yeah, it gets rid of the warning, but now all the dirty tracking tests fail. I guess I'll have to dig deeper.

[sgrif]

Yeah I'm looking at it now, too. It looks like prior to this, we were manually defining the _changed? method -- I think that's the right thing to do here, for this to work properly (especially with mutation).

[rafaelfranca]

Fixed on master