New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add `update_heritable_value_of` #39487

Open

jonathanhefner wants to merge 1 commit into rails:main from jonathanhefner:add-class_store

+126 −15

Member

jonathanhefner commented May 30, 2020 •

edited

Using a class_attribute for inheritance of values in a Hash is tricky. When storing a new value in a subclass's Hash, you must be careful to not modify the superclass's Hash. On the flip side, when storing a new value in a superclass's Hash, you must ensure the subclass can access the value if and only if it has not already been overridden in the subclass's Hash. (See #39467, #39468, and #39473 for examples of addressing the latter.)

update_heritable_value_of handles both of these requirements:

A = Class.new
B = Class.new(A)
C = Class.new(B)

A.class_attribute :settings, default: {}

A.update_heritable_value_of(:settings, :foo, "A")
A.update_heritable_value_of(:settings, :bar, "A")
B.update_heritable_value_of(:settings, :bar, "B")
C.update_heritable_value_of(:settings, :qux, "C")
A.update_heritable_value_of(:settings, :qux, "A")

A.settings  # == {:foo=>"A", :bar=>"A", :qux=>"A"}
B.settings  # == {:foo=>"A", :bar=>"B", :qux=>"A"}
C.settings  # == {:foo=>"A", :bar=>"B", :qux=>"C"}

/cc @eugeneius
/cc @kamipo

This is an alternative approach to what was being discussed in #39467 (comment) and #39467 (comment).

rails-bot bot added the activesupport label

jonathanhefner force-pushed the add-class_store branch from ddd4e9c to d23ee6a Compare

May 31, 2020 01:56

eugeneius closed this

eugeneius reopened this

Member

eugeneius commented May 31, 2020

(Whoops!)

It's hard to reason about this whether this is useful on its own - do you think you could convert some code that currently uses class_attribute to use this instead as an example?

jonathanhefner force-pushed the add-class_store branch from d23ee6a to fe7fee1 Compare

May 31, 2020 05:40

rails-bot bot added actionmailer activemodel activerecord activestorage labels

jonathanhefner force-pushed the add-class_store branch 2 times, most recently from 4f316a9 to 05915a9 Compare

May 31, 2020 05:55

Member Author

jonathanhefner commented May 31, 2020 •

edited

@eugeneius Sure! I added some additional commits to showcase its usage.

~~The tests are currently failing because one of the commits does not preserve the Hash iteration order from this line. I'll give some thought to how to accommodate that.~~

jonathanhefner force-pushed the add-class_store branch 2 times, most recently from ece3b0d to 6cd7151 Compare

June 4, 2020 05:51

jonathanhefner force-pushed the add-class_store branch from 6cd7151 to aed5d6d Compare

June 13, 2020 04:43

kamipo added a commit to kamipo/rails that referenced this pull request


          Extract inplace_updatable_class_attribute

5f0e546

I've partly picked `LiloHash` from rails#39487.

Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>

jonathanhefner force-pushed the add-class_store branch 2 times, most recently from 0fcd6cf to b4451ea Compare

June 14, 2020 07:37

Member

rafaelfranca commented Jun 16, 2020

Can we avoid the dynamic method generation? It is hard to document and find documentation to it. Instead of update_delivery_methods_with_heritable_value can we use update_with_heritable_value(:delivery_methods)? This way we can document update_with_heritable_value and if you search where that method is defined it is easy to find.

jonathanhefner force-pushed the add-class_store branch 4 times, most recently from a3d6875 to b7cf5ba Compare

June 16, 2020 21:43

jonathanhefner changed the title ~~Add Class::class_store~~ Add update_heritable_value_of

eugeneius mentioned this pull request

ActiveRecord::Enum and inheritance bug #39756

Closed

This was referenced Sep 3, 2020

Ensure attributes are always inherited #39467

Closed

Ensure attribute decorations are always inherited #39468

Closed

Member Author

jonathanhefner commented Sep 3, 2020

@rafaelfranca Does the current API in the PR description look all right? If so, I can begin squashing, rebasing, writing tests, etc.

jonathanhefner mentioned this pull request

Unify decorate_attribute_type and attribute #39929

Merged

Base automatically changed from master to main

January 14, 2021 17:01

rails-bot bot commented Apr 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

rails-bot bot added the stale label

jonathanhefner force-pushed the add-class_store branch from 72b4dc3 to 8c2ee61 Compare

April 19, 2021 16:15

rails-bot bot removed the stale label

jonathanhefner force-pushed the add-class_store branch 3 times, most recently from 2e9006e to 1fbcad6 Compare

April 19, 2021 17:41

jonathanhefner marked this pull request as ready for review

April 19, 2021 17:57

jonathanhefner force-pushed the add-class_store branch from 1fbcad6 to 54a789e Compare

April 19, 2021 18:15

rails-bot bot commented Jul 18, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

rails-bot bot added the stale label

jonathanhefner force-pushed the add-class_store branch from 54a789e to fd20342 Compare

July 19, 2021 15:08

rails-bot bot removed the stale label

rails-bot bot commented Oct 17, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

rails-bot bot added the stale label

jonathanhefner force-pushed the add-class_store branch from fd20342 to d84bc77 Compare

October 18, 2021 15:44

rails-bot bot removed the stale label

rails-bot bot commented Jan 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

rails-bot bot added the stale label

jonathanhefner force-pushed the add-class_store branch from d84bc77 to 3c08003 Compare

January 20, 2022 20:46

rails-bot bot removed the stale label

rails-bot bot commented Apr 20, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

rails-bot bot added the stale label

rails-bot bot closed this

jonathanhefner reopened this

rails-bot bot removed the stale label


          Add update_heritable_value_of

1d48ef6

Using a `class_attribute` for inheritance of values in a Hash is tricky.
When storing a new value in a subclass's Hash, you must be careful to
not modify the superclass's Hash.  On the flip side, when storing a new
value in a superclass's Hash, you must ensure the subclass can access
the value if and only if it has not already been overridden in the
subclass's Hash.

`update_heritable_value_of` handles both of these requirements:

```ruby
A = Class.new
B = Class.new(A)
C = Class.new(B)

A.class_attribute :settings, default: {}

A.update_heritable_value_of(:settings, :foo, "A")
A.update_heritable_value_of(:settings, :bar, "A")
B.update_heritable_value_of(:settings, :bar, "B")
C.update_heritable_value_of(:settings, :qux, "C")
A.update_heritable_value_of(:settings, :qux, "A")

A.settings # => {:foo=>"A", :bar=>"A", :qux=>"A"}
B.settings # => {:foo=>"A", :bar=>"B", :qux=>"A"}
C.settings # => {:foo=>"A", :bar=>"B", :qux=>"C"}
```

jonathanhefner force-pushed the add-class_store branch from 3c08003 to 1d48ef6 Compare

April 28, 2022 16:23

jonathanhefner changed the title ~~Add update_heritable_value_of~~ Add update_heritable_value_of

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment