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 option for class_attribute default #29270

Merged
merged 5 commits into from May 29, 2017
Merged

Add option for class_attribute default #29270

merged 5 commits into from May 29, 2017

Conversation

@dhh
Copy link
Member

dhh commented May 29, 2017

It's extremely common, as evidenced by the commit in this PR of Rails conversions, to set a default value for a class_attribute. So let's provide that directly.

class_attribute :_layout, :_layout_conditions, instance_accessor: false
self._layout = nil
self._layout_conditions = {}
class_attribute :_layout

This comment has been minimized.

Copy link
@matthewd

matthewd May 29, 2017

Member

Lost instance_accessor: false

@@ -123,6 +124,10 @@ def class_attribute(*attrs)
remove_possible_method "#{name}="
attr_writer name
end

if default_value

This comment has been minimized.

Copy link
@matthewd

matthewd May 29, 2017

Member

unless default_value.nil?

This comment has been minimized.

Copy link
@matthewd

matthewd May 29, 2017

Member

.. or just change the nil on line 79/80

@@ -53,9 +53,8 @@ class << self; attr_accessor :helpers_path; end
include AbstractController::Helpers

included do
class_attribute :helpers_path, :include_all_helpers
self.helpers_path ||= []

This comment has been minimized.

Copy link
@dhh

dhh May 29, 2017

Author Member

Probably shouldn't replace this with a default:, right? Given that it checks the parent class first.

This comment has been minimized.

Copy link
@matthewd

matthewd May 29, 2017

Member

I hesitated at that, but ultimately convinced myself the ||= is a lie: class_attribute will always define a method on the current class, with the value set to nil... so AFAICS, this is no different from if it were a straight assignment.

This comment has been minimized.

Copy link
@dhh

dhh May 29, 2017

Author Member

Ah, right. Yup, good 👍

@@ -62,8 +62,7 @@ module Callbacks

included do
extend ActiveSupport::DescendantsTracker
class_attribute :__callbacks, instance_writer: false
self.__callbacks ||= {}

This comment has been minimized.

Copy link
@dhh

dhh May 29, 2017

Author Member

Same issue here: "Probably shouldn't replace this with a default:, right? Given that it checks the parent class first."

instance_predicate = options.fetch(:instance_predicate, true)
default_value = options.fetch(:default, nil)

This comment has been minimized.

Copy link
@matthewd

matthewd May 29, 2017

Member

This (really, the fact it's in the options hash at all) does mean the methods below will capture the default value in their closure -- so even if the value is changed on the root class, the original default won't be GCed.

I don't think we particularly need to care about a couple of extra empty hashes and arrays floating around, but I figured I'd at least mention it.

This comment has been minimized.

Copy link
@dhh

dhh May 29, 2017

Author Member

Would be nice to convert all these to kwargs. Should probably do a big sweep for Rails 6.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca May 30, 2017

Member

We can do it right now. We only support Ruby versions that work with kwargs

@@ -70,9 +70,10 @@ class Class
# To opt out of both instance methods, pass <tt>instance_accessor: false</tt>.
def class_attribute(*attrs)
options = attrs.extract_options!
instance_reader = options.fetch(:instance_accessor, true) && options.fetch(:instance_reader, true)
instance_writer = options.fetch(:instance_accessor, true) && options.fetch(:instance_writer, true)
instance_reader = options.fetch(:instance_accessor, true) && options.fetch(:instance_reader, true)

This comment has been minimized.

Copy link
@simi

simi May 29, 2017

Contributor

Sorry for a little off-topic here: I have spotted this formatting changes in few recent PRs. Is this new standard? Wasn't this considered "cosmetic" change before?

@dhh
Copy link
Member Author

dhh commented May 29, 2017

@dhh dhh merged commit 1c275d8 into master May 29, 2017
0 of 3 checks passed
0 of 3 checks passed
codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@dhh dhh deleted the class-attribute-default branch May 29, 2017
@gsamokovarov
Copy link
Contributor

gsamokovarov commented May 29, 2017

David, have you tried defaulting with a block?

class_attribute :periodic_timers, instance_reader: false, default: []

vs

class_attribute(:periodic_timers, instance_reader: false) { [] }

The reason I'm bringing it up is because mattr_accessor works like 👆 and does not have the default option. Should we pair the functionality so it's symmetrical across all the attribute extensions?

@dhh
Copy link
Member Author

dhh commented May 29, 2017

That strikes me as less revealing, but the upside is that it won't get evaluated until you access it, which is a nice perk. Default does have symmetry with belongs_to/default, though.

@gsamokovarov
Copy link
Contributor

gsamokovarov commented May 29, 2017

Add a default to mattr_accessor as well then? Dunno if it will make the interface too big though.

@dhh
Copy link
Member Author

dhh commented May 29, 2017

@dhh
Copy link
Member Author

dhh commented May 30, 2017

@gsamokovarov
Copy link
Contributor

gsamokovarov commented May 30, 2017

mattr_accessor family actually accept options as well, they let you choose whether or not to have the instance methods. The interfaces between class_attributes and cattr_accessor aren't that far off.

@kaspth
Copy link
Member

kaspth commented May 30, 2017

Let's make it so!

@dhh GitHub didn't connect your email reply: this was in reference to kwargs across the board right?

@dhh
Copy link
Member Author

dhh commented May 30, 2017

@dhh
Copy link
Member Author

dhh commented May 30, 2017

@kaspth
Copy link
Member

kaspth commented May 30, 2017

Looks like mattr_* just yields to the block immediately, so there's no lazy loading at all:

class_variable_set("@@#{sym}", yield) if block_given?

Then there doesn't seem to be a need for default: to take lambdas — besides all our class_attribute didn't need deferred evaluation, and I haven't seen the need for it in my app's usage of mattr_* either.

I'd be 👍 on deprecating the block to mattr_* and cattr_* and replacing it with a default: arg.

@dhh
Copy link
Member Author

dhh commented May 30, 2017

@gsamokovarov
Copy link
Contributor

gsamokovarov commented May 30, 2017

Cool, I can take stab at that.

jbourassa pushed a commit to jbourassa/rails that referenced this pull request Aug 24, 2017
PR rails#29270 changed the number of arguments that gets passed to Procs
defined in ActionMail::Base.default.

This fixes it and introduce a deprecation so the hack can be removed
(the argument is useless since it has the same value as `self`).
jbourassa pushed a commit to jbourassa/rails that referenced this pull request Aug 29, 2017
PR rails#29270 changed the number of arguments that gets passed to Procs
defined in ActionMail::Base.default. With this changeset, Procs can
now have 1 or 0 arguments

Also adds test coverage for AM::Base.default Proc arity.
antonzaytsev added a commit to sdtechdev/activestorage-rails4 that referenced this pull request Dec 19, 2017
As default value for class_attribute introduced only in Rails 5
rails/rails#29270
matthewrudy pushed a commit to matthewrudy/rude-rails that referenced this pull request Feb 12, 2018
* Allow a default value to be declared for class_attribute

* Convert to using class_attribute default rather than explicit setter

* Removed instance_accessor option by mistake

* False is a valid default value

* Documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.