Skip to content
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.

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

Add custom prefix to ActiveRecord::Store accessors #32306

Merged
merged 1 commit into from Mar 24, 2018

Conversation

danhuynhdev
Copy link
Contributor

@danhuynhdev danhuynhdev commented Mar 20, 2018

Add a prefix option to ActiveRecord::Store.store_accessor and
ActiveRecord::Store.store. This option allows stores to have identical keys
with different accessors.

Summary

I often bump into problems at work when using ActiveRecord::Store because of name collision of
accessors in a store. Consider this snippet of code:

class RequestOff < ApplicationRecord
  store :from, accessors: [:date, :period], coder: JSON
  store :to, accessors: [:date, :period], coder: JSON
end

The accessors above will not work because there are two accessors with the same name. To solve the
problems I either have to change the keys in the store or write my own accessors. The new prefix option
will fix the problem above by adding a unique prefix to the accessor without changing the underlying key.
Example:

class RequestOff < ApplicationRecord
  store :from, accessors: [:date, :period], coder: JSON, prefix: true
  store :to, accessors: [:date, :period], coder: JSON, prefix: :to
end

ro = RequestOff.new
ro.from_date = "2018-11-1"

Copy link
Contributor

@pixeltrix pixeltrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks for the PR

@@ -31,10 +31,14 @@ module ActiveRecord
#
# class User < ActiveRecord::Base
# store :settings, accessors: [ :color, :homepage ], coder: JSON
# store :mother, accessors: [ :name ], coder: JSON, prefix: true
# store :lover, accessors: [ :name ], coder: JSON, prefix: :darling
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to:

#     store :parent, accessors: [ :name ], coder: JSON, prefix: true
#     store :spouse, accessors: [ :name ], coder: JSON, prefix: :partner

and obviously update the example below.

@@ -44,6 +48,7 @@ module ActiveRecord
# # Add additional accessors to an existing store through store_accessor
# class SuperUser < User
# store_accessor :settings, :privileges, :servants
# store_accessor :mother, :birthday, prefix: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - :parent instead of :mother

"#{prefix == true ? store_attribute : prefix}_"
else
""
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might look better as:

accessor_prefix =
  case prefix
  when String, Symbol
    "#{prefix}_"
  when TrueClass
    "#{store_attribute}_"
  else
    ""
  end

it makes the intent clearer than hiding inside a ternary in a string interpolation.

name: "John Doe", color: "black", remember_login: true,
height: "tall", is_a_good_guy: true,
mother_name: "Mary", darling_name: "Lily",
darling_birthday: "1997-11-1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the attribute names to be gender neutral as in the documentation comment above and the rest of the tests as well.

@danhuynhdev danhuynhdev force-pushed the feature/store-accessor-prefix branch 3 times, most recently from 5a790c3 to aa4ed77 Compare March 21, 2018 01:30
@danhuynhdev
Copy link
Contributor Author

Fixed

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a prefix option to ActiveRecord::Store.store_accessor and
ActiveRecord::Store.store. This option allows stores to have identical keys
with different accessors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants