Skip to content
Browse files

ActiveRecord::Base#new_record? now returns false for existing records…

… (was nil) [#1219 state:committed]

Signed-off-by: David Heinemeier Hansson <david@loudthinking.com>
  • Loading branch information...
1 parent 4f043a4 commit 6e98adfc8e19a39fa45d4acd94145d318d151964 @yaroslav yaroslav committed with dhh Dec 27, 2008
Showing with 9 additions and 2 deletions.
  1. +2 −0 activerecord/CHANGELOG
  2. +2 −2 activerecord/lib/active_record/base.rb
  3. +5 −0 activerecord/test/cases/base_test.rb
View
2 activerecord/CHANGELOG
@@ -1,5 +1,7 @@
*2.3.0/3.0*
+* Fixed that ActiveRecord::Base#new_record? should return false (not nil) for existing records #1219 [Yaroslav Markin]
+
* I18n the word separator for error messages. Introduces the activerecord.errors.format.separator translation key. #1294 [Akira Matsuda]
* Add :having as a key to find and the relevant associations. [Emilio Tagua]
View
4 activerecord/lib/active_record/base.rb
@@ -2406,9 +2406,9 @@ def id=(value)
write_attribute(self.class.primary_key, value)
end
- # Returns true if this object hasn't been saved yet -- that is, a record for the object doesn't exist yet.
+ # Returns true if this object hasn't been saved yet -- that is, a record for the object doesn't exist yet; otherwise, returns false.
def new_record?
- defined?(@new_record) && @new_record
+ (defined?(@new_record) && @new_record) || false
end
# :call-seq:
View
5 activerecord/test/cases/base_test.rb
@@ -1198,6 +1198,11 @@ def test_boolean_cast_from_string
assert b_true.value?
end
+ def test_new_record_returns_boolean
+ assert_equal Topic.new.new_record?, true
+ assert_equal Topic.find(1).new_record?, false
+ end
+
def test_clone
topic = Topic.find(1)
cloned_topic = nil

21 comments on commit 6e98adf

@al2o3cr

Maybe I’m missing something, but isn’t returning nil essentially equivalent to returning false? Both will be handled the same in an @if o.new_record? @ context..

This also doesn’t appear to address the original issue raised by ticket #1219.

Finally, the succeeding patch (remove defined?) will raise a warning when run in -w mode. I’m guessing that’s why the code was written that way originally…

@supaspoida

While it is equivalent, the specific issue I encountered was the (possibly wrong) expectation that a method ending in ? should return either true or false. Sure Ruby evaluates nil & false the same, but if you are explicitly checking against false, getting a nil value back will not give you the expected results.

The original issue in 1219 seemed to be related to the new_record? issue, as I’m guessing somewhere rails was also expecting a false value but getting nils. But you are correct this fix does not necessarily mean the original issue has been fully addressed.

@xaviershay

new_record? not returning false has tripped me up a few times. +1 this change.

@drnic
drnic commented on 6e98adf Dec 27, 2008

methods ending in ? don’t have to return true or false afaik. The ? infers that the response can be used as such.

@fxn
Ruby on Rails member
fxn commented on 6e98adf Dec 27, 2008

From Flanagan & Matz (section 6.2. Method Names): “Predicates typically return one of the Boolean values true or false, but this is not required, as any value other than false or nil works like true when a Boolean value is required. (The Numeric method nonzero?, for example, returns nil if the number it is invoked on is zero, and just returns the number otherwise.)”

@michaelklishin

They don’t have to, but a lot of people expect them to :)

@fxn
Ruby on Rails member
fxn commented on 6e98adf Dec 27, 2008

But it is a wrong expectation :-), a predicate returns a boolean value, it is just a test. Any value may act as a boolean value in Ruby.

That’s why in general you don’t test against == (false|true), you just use the value returned by the predicate in boolean context.

@xaviershay

“Predicates typically return”

Typical, principle of least surprise. Show me some code where treating the return value of a predicate as anything other than a boolean is a good idea.

@al2o3cr

superspoida: the issue in 1219 was that records returned (I’m guessing – no code was provided) from a find_by_sql with a right join clause weren’t being marked as new. This doesn’t address that, as those records will now return false rather than nil, but not true as 1219 wanted them to. Quite frankly, the case in 1219 was sufficiently obscure that it probably shouldn’t be handled automatically.

And I’d definitely agree with fxn – if you see @ == true @ or @ == false @ in Ruby code, someone is Doing It Wrong.

@fxn
Ruby on Rails member
fxn commented on 6e98adf Dec 27, 2008

@xaviershay they typically return the singletons true/false, but not always. Ruby has clear semantics for boolean values: false and nil are booleanly false, any other object is booleanly true.

So, you say

    do_this if some_predicate?

and you don’t care what type of object some_predicate? returns, in Ruby you care about its boolean interpretation.

@al2o3cr

argh – textile formatting ate my == signs…
It should have been == true and == false above.

@oggy
oggy commented on 6e98adf Jan 2, 2009

One situation where returning nil instead of false can trip you is if you’re comparing two predicates: if foo? == bar?. If one’s nil, and one’s false, you lose, and I don’t think such usage is a code smell.

drnic is right in that not all ?-methods return true/false, but as far as I know such instances are always explicitly documented in standard ruby.

+1 for this.

@henrik
henrik commented on 6e98adf Jan 2, 2009

As I understand it, ?-methods that return `nil` as their “falsy” value return something other than `true` as their truthy value. `nonzero?` returns a number; `defined?` returns a string like “constant”.

So `new_record?` returning `nil` and `true` was definitely unidiomatic, and the change makes good sense.

@josevalim
Ruby on Rails member

The changes make sense for me also. In rspec, I would write:

user_one.new_record?.should be_true
user_two.new_record?.should be_false

When I write:

user_three.new_record?.should be_nil

Seems smelly!

@fxn
Ruby on Rails member
fxn commented on 6e98adf Jan 2, 2009

george-ogata but that test is broken! @foo? could return 0 and bar? return 1 or "bar".

Equality has a meaning in Ruby, it compares objects in a certain way. Interpretation of values as booleans is a different business. You just do not compare boolean equivalence with == in Ruby.

If some code expects a precise value, for example filters halted returning exactly false some versions ago, then you compare them that way. Otherwise you need to play by the rules of booleans in Ruby.

@fxn
Ruby on Rails member
fxn commented on 6e98adf Jan 2, 2009

josevalim: you need to test the boolean meaning of new_record?. The test shouldn’t be looking specifically for nil.

In fact, note that the documentation of new_record? doesn’t even mention a keyword. It only says that returns true (English true, which is not necessarily the same as true) if such and such.

Problem in your example lies in RSpec usage itself. I am not into RSpec, but my understanding is that be_false translates to == false so to speak. If that’s the case, that is not a test for predicates. It tests object equality, which is stronger than the contract of the predicate. I think you test for (English) true simply with .should be.

Of course if you were the developer of the very predicate you could be interested in testing the exact expected value, not only its boolean interpretation, but that’s a different story.

@joshsusser

josevalim, fxn: to test boolean truthiness, as opposed to the identity being == true or false, you can use the rspec #be matcher.

new_rec.new_record?.should be
old_rec.new_record?.should_not be

@bterlson

Personally I prefer using the custom predicate matchers…

new_rec.should be_a_new_record
old_rec.should_not be_a_new_record

Which should work with/without this commit afaik.

@fxn
Ruby on Rails member
fxn commented on 6e98adf Jan 2, 2009

bterlson: sounds good.

Point is, a predicate returns either a true or false value. In the case of new_record? the actual returned object is unespecified, and the predicate works correctly.

In that sense I am -1 on this patch.

@xaviershay

Try approaching it from the other direction

the_car = car.new_record?
the_car.drive!

Does this look like bad code to you?
Should the framework encourage this?
Or should the framework behave in a “typical” manner?

For a more concrete example, principle of least surprise says the following should halt the filter chain:

before_filter do
  return @object.new_record?
end
@fxn
Ruby on Rails member
fxn commented on 6e98adf Jan 2, 2009

xaviershay: the framework should use and encourage idiomatic Ruby.

It is not idiomatic Ruby to expect any particular object from predicates. In particular there’s no ground on expecting the singletons true/false. You may not like that, but Ruby is designed that way.

Please sign in to comment.
Something went wrong with that request. Please try again.