Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Deprecate ActiveRecord#Base.default_scopes? #10113

Merged
merged 2 commits into from

4 participants

Agis Anastasopoulos Rafael Mendonça França Carlos Antonio da Silva John J. Wang
Agis Anastasopoulos

Fixes #10107.

Agis Anastasopoulos

So what do you think? @rafaelfranca

guides/source/active_support_core_extensions.md
@@ -1039,6 +1039,8 @@ For convenience `class_attribute` also defines an instance predicate which is th
When `:instance_reader` is `false`, the instance predicate returns a `NoMethodError` just like the reader method.
+If you do not want the instance predicate, pass `:skip_instance_predicate => true` and it wonill not be defined.
John J. Wang
wangjohn added a note

Typo here, it should probably be "it will not be defined."

Agis Anastasopoulos
agis- added a note

Fixed, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...upport/lib/active_support/core_ext/class/attribute.rb
@@ -75,7 +76,7 @@ def class_attribute(*attrs)
attrs.each do |name|
define_singleton_method(name) { nil }
- define_singleton_method("#{name}?") { !!public_send(name) }
+ define_singleton_method("#{name}?") { !!public_send(name) } unless options[:skip_instance_predicate]
Rafael Mendonça França Owner

I think is better to use the same convention and call this option instance_predicate

Agis Anastasopoulos
agis- added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/lib/active_record/scoping/default.rb
@@ -5,7 +5,8 @@ module Default
included do
# Stores the default scope for the class.
- class_attribute :default_scopes, instance_writer: false
+ class_attribute :default_scopes, instance_writer: false,
+ skip_instance_predicate: true
Rafael Mendonça França Owner

We will need to deprecated the instance predicate here. We don't know how use it right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Rafael Mendonça França
Owner

@Agis- very good. I made some comments.

Thank you

...upport/lib/active_support/core_ext/class/attribute.rb
@@ -72,10 +73,11 @@ def class_attribute(*attrs)
# double assignment is used to avoid "assigned but unused variable" warning
instance_reader = 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_predicate = !options.fetch(:skip_instance_predicate, false)
Rafael Mendonça França Owner

I'm talking about changing the key name to instance_predicate like the others, with default to true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
guides/source/active_support_core_extensions.md
@@ -1039,6 +1039,8 @@ For convenience `class_attribute` also defines an instance predicate which is th
When `:instance_reader` is `false`, the instance predicate returns a `NoMethodError` just like the reader method.
+If you do not want the instance predicate, pass `:skip_instance_predicate => true` and it will not be defined.

Make sure to use 1.9 style hash throughout your changes ;)

Agis Anastasopoulos
agis- added a note

Cool :) Even in tests where all the other cases are using the old style?

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

@rafaelfranca Updated :)

Rafael Mendonça França
Owner

Seems good. Could you rebase your branch and squash your commits?

Rafael Mendonça França rafaelfranca merged commit a9da549 into from
Rafael Mendonça França
Owner

Thank you

Agis Anastasopoulos agis- deleted the branch
Carlos Antonio da Silva carlosantoniodasilva commented on the diff
activerecord/lib/active_record/scoping/default.rb
((7 lines not shown))
self.default_scopes = []
+
+ def self.default_scopes?
+ ActiveSupport::Deprecation.warn(
+ "#default_scopes? is deprecated. Do something like #default_scopes.empty? instead."
+ )
+
+ !!self.default_scopes
+ end

What if we fixed this implementation to check for default_scopes.empty? instead?

Agis Anastasopoulos
agis- added a note

Then that would make more sense :) I'll open a PR.

Rafael Mendonça França Owner

I'd not do this. I think we should discourage this method, not implement the proper behavior, since it only exist by mistake.

Agis Anastasopoulos
agis- added a note

Really can't make my mind on this, both opinions are valid. Btw, the PR: #10129.

Alright, now I wonder how many mistakes we have out there ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
5 activerecord/CHANGELOG.md
View
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* `#default_scopes?` is deprecated. Instead, do something like
+ `Post.default_scopes.empty?`.
+
+ *Agis Anastasopoulos*
+
* Default values for PostgreSQL bigint types now get parsed and dumped to the
schema correctly.
11 activerecord/lib/active_record/scoping/default.rb
View
@@ -5,8 +5,17 @@ module Default
included do
# Stores the default scope for the class.
- class_attribute :default_scopes, instance_writer: false
+ class_attribute :default_scopes, instance_writer: false, instance_predicate: false
+
self.default_scopes = []
+
+ def self.default_scopes?
+ ActiveSupport::Deprecation.warn(
+ "#default_scopes? is deprecated. Do something like #default_scopes.empty? instead."
+ )
+
+ !!self.default_scopes
+ end

What if we fixed this implementation to check for default_scopes.empty? instead?

Agis Anastasopoulos
agis- added a note

Then that would make more sense :) I'll open a PR.

Rafael Mendonça França Owner

I'd not do this. I think we should discourage this method, not implement the proper behavior, since it only exist by mistake.

Agis Anastasopoulos
agis- added a note

Really can't make my mind on this, both opinions are valid. Btw, the PR: #10129.

Alright, now I wonder how many mistakes we have out there ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
module ClassMethods
6 activesupport/CHANGELOG.md
View
@@ -1,5 +1,11 @@
## Rails 4.0.0 (unreleased) ##
+* `Class#class_attribute` accepts an `instance_predicate` option which
+ defaults to `true`. If set to `false` the predicate method will not
+ be defined.
+
+ *Agis Anastasopoulos*
+
* `fast_xs` support has been removed. Use 'String#encode(xml: :attr)`.
* `ActiveSupport::Notifications::Instrumenter#instrument` should yield
8 activesupport/lib/active_support/core_ext/class/attribute.rb
View
@@ -44,7 +44,8 @@ class Class
# Base.setting # => []
# Subclass.setting # => [:foo]
#
- # For convenience, a query method is defined as well:
+ # For convenience, an instance predicate method is defined as well.
+ # To skip it, pass <tt>instance_predicate: false</tt>.
#
# Subclass.setting? # => false
#
@@ -72,10 +73,11 @@ def class_attribute(*attrs)
# double assignment is used to avoid "assigned but unused variable" warning
instance_reader = 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_predicate = options.fetch(:instance_predicate, true)
attrs.each do |name|
define_singleton_method(name) { nil }
- define_singleton_method("#{name}?") { !!public_send(name) }
+ define_singleton_method("#{name}?") { !!public_send(name) } if instance_predicate
ivar = "@#{name}"
@@ -109,7 +111,7 @@ def class_attribute(*attrs)
self.class.public_send name
end
end
- define_method("#{name}?") { !!public_send(name) }
+ define_method("#{name}?") { !!public_send(name) } if instance_predicate
end
attr_writer name if instance_writer
9 activesupport/test/core_ext/class/attribute_test.rb
View
@@ -27,7 +27,7 @@ def setup
assert_equal 1, Class.new(@sub).setting
end
- test 'query method' do
+ test 'predicate method' do
assert_equal false, @klass.setting?
@klass.setting = 1
assert_equal true, @klass.setting?
@@ -48,7 +48,7 @@ def setup
assert_equal 1, object.setting
end
- test 'instance query' do
+ test 'instance predicate' do
object = @klass.new
assert_equal false, object.setting?
object.setting = 1
@@ -73,6 +73,11 @@ def setup
assert_raise(NoMethodError) { object.setting = 'boom' }
end
+ test 'disabling instance predicate' do
+ object = Class.new { class_attribute :setting, instance_predicate: false }.new
+ assert_raise(NoMethodError) { object.setting? }
+ end
+
test 'works well with singleton classes' do
object = @klass.new
object.singleton_class.setting = 'foo'
2  guides/source/active_support_core_extensions.md
View
@@ -1039,6 +1039,8 @@ For convenience `class_attribute` also defines an instance predicate which is th
When `:instance_reader` is `false`, the instance predicate returns a `NoMethodError` just like the reader method.
+If you do not want the instance predicate, pass `instance_predicate: false` and it will not be defined.
+
NOTE: Defined in `active_support/core_ext/class/attribute.rb`
#### `cattr_reader`, `cattr_writer`, and `cattr_accessor`
Something went wrong with that request. Please try again.