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

Introduce mattr_accessor default option #29294

Merged
merged 2 commits into from
Jun 4, 2017

Conversation

gsamokovarov
Copy link
Contributor

@gsamokovarov gsamokovarov commented May 31, 2017

Since David introduced a default: to the class_attribute macro, we decided to follow it in the mattr_accessor family of methods. This PR does not deprecate the current defaulting behaviour:

mattr_accessor(:settings) { {} }

I simply do not document it anymore. My fear is that it may be too disruptive and force plugin authors to change their code, while they can go without that, but if you still prefer to properly deprecate it, I can issue the warning.

@rails-bot
Copy link

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

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

default ||= yield if block_given?
class_variable_set("@@#{sym}", default) if default
Copy link
Contributor

@kaspth kaspth May 31, 2017

Choose a reason for hiding this comment

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

false needs to be an acceptable default, so unless default.nil? to match class_attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch!

@gsamokovarov
Copy link
Contributor Author

gsamokovarov commented May 31, 2017

The failure seems a sporadic AC one.

Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

Let's squash the commits down to two: one for the new implementation with kwargs and then another to switch the codebase to use :default.

def mattr_reader(*syms)
options = syms.extract_options!
def mattr_reader(*syms, instance_reader: nil, instance_accessor: nil, default: nil)
default ||= yield if block_given?
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to have odd rules of precedence. Consider:

mattr_accessor(:hair_colors, default: false) { [ :brown ] } # default would be brown.
mattr_accessor(:hair_colors, default: true)  { [ :brown ] } # default would be true.

These aren't supposed to mix of course. But I'd like default to always take precedence since it's the newest option and more intention revealing of the two. So:

default = yield if block_given? && default.nil?

Then same in mattr_writer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also move it to the end of the method, so the default handling isn't spread across the method. Same for the writer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to the top, because of the loop. Won't happen often, but when we pass multiple attributes, we'll waste some time that way.

# end
#
# class Person
# include HairColors
# end
#
# Person.class_variable_get("@@hair_colors") # => [:brown, :black, :blonde, :red]
def mattr_writer(*syms)
options = syms.extract_options!
def mattr_writer(*syms, instance_writer: nil, instance_accessor: nil, default: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we default instance_writer and instance_accessor to true here could we then trim the == false below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now seems like a good time to try it. Doubt someone used an explicit instance_writer: nil.

mattr_accessor :properties
class << (@@properties = [])
mattr_accessor :properties, default: []
class << @@properties
Copy link
Contributor

Choose a reason for hiding this comment

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

New line before this one.


While the example above still works, the recommended way to set a default now is:

mattr_accessor :settings, default: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just mention the new way only and spell out the methods.

*   Add default option to module and class attribute accessors.

       mattr_accessor :settings, default: {}

    Works for `mattr_reader`, `mattr_writer`, `cattr_accessor`, `cattr_reader`, and `cattr_writer` as well.

def mattr_reader(*syms)
options = syms.extract_options!
def mattr_reader(*syms, instance_reader: true, instance_accessor: true, default: nil)
default = yield if block_given? && default.nil?
Copy link
Member

Choose a reason for hiding this comment

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

Moving the yield out of the loop is a behavioural change.. probably better avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it back in.

@gsamokovarov gsamokovarov force-pushed the attributes-default branch 3 times, most recently from e4dd52c to 524b677 Compare June 2, 2017 14:20
class_eval(<<-EOS, __FILE__, __LINE__ + 1)
def #{sym}
@@#{sym}
end
EOS
end
class_variable_set("@@#{sym}", yield) if block_given?

default = yield if block_given? && default.nil?
Copy link
Member

Choose a reason for hiding this comment

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

This still only yields once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had a test for that even before, see this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But actually, looking at the test, it was pretty bad to begin with. It doesn't exercise the multiple invocation. 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it and added a test for the multiple yield.

@gsamokovarov
Copy link
Contributor Author

I think I have cleaned the history and addressed all the comments. Can you guys go over this one again?

@gsamokovarov gsamokovarov force-pushed the attributes-default branch 2 times, most recently from 6391db2 to 55fb1c4 Compare June 2, 2017 15:48
@kaspth kaspth merged commit 704209b into rails:master Jun 4, 2017
@kaspth
Copy link
Contributor

kaspth commented Jun 4, 2017

Well done 👏

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Apr 3, 2018
…iled_template_methods`

Since we introduced default option for `class_attribute` and
`mattr_accessor` family of methods and changed all occurrences of setting
default values by using of `:default` option I think it would be fine to use
`:default` option in order to set default value of `finalize_compiled_template_methods`
since it expresses itself very well.

Related to rails#29294, rails#32418
kamipo added a commit that referenced this pull request Oct 20, 2018
Since #29294, `mattr_acessor` uses kwargs for `instance_reader`,
`instance_writer`, and `instance_accessor` options.

`thread_mattr_accessor` and `config_accessor` also take the same
options, so let's maintain these options handles the same.
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Mar 12, 2022
This reverts commit 212463d, reversing
changes made to 2837572.

Though `mattr_*` methods do still accept a block, documentation for that
usage was removed in rails#29294 as way to soft-deprecate in favor of passing
a `default:` kwarg.
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.

5 participants