Bug #7437

Array#delete(obj) should return obj when there is an object that is equal in the array

Added by Hiro Asari over 2 years ago. Updated over 2 years ago.

[ruby-core:50145]
Status:Closed
Priority:Normal
Assignee:Marc-Andre Lafortune
ruby -v:ruby 2.0.0dev (2012-11-25 trunk 37839) [x86_64-darwin12.2.1] Backport:

Description

According to http://www.ruby-doc.org/core-1.9.3/Array.html#method-i-delete, Array#delete(obj) should return "obj" when there are objects in the array that are "equal to obj" (internally, "==" is used, it seems).

Notice that the documentation does not state that the return value is an element of the array itself. However, 1.9.3 and trunk both return a member of the Array, rather than the argument.

This issue was raised in https://github.com/jruby/jruby/issues/411

#!/usr/bin/env ruby

class Foo
attr_reader :name, :age

def initialize name, age
@name = name
@age = age
end

def == other
other.name == name
end
end

foo1 = Foo.new "John Shahid", 27
foo2 = Foo.new "John Shahid", 28
array = [foo1]
temp = array.delete foo2 # => foo1, not foo2

ruby-7437.patch Magnifier (522 Bytes) Hiro Asari, 11/26/2012 01:21 PM

Associated revisions

Revision 37872
Added by Marc-Andre Lafortune over 2 years ago

  • array.c: Fix rdoc for Array#delete [#7437]

Revision 37872
Added by Marc-Andre Lafortune over 2 years ago

  • array.c: Fix rdoc for Array#delete [#7437]

History

#1 Updated by Hiro Asari over 2 years ago

Here's the patch. Where should the tests go? RubySpec?

#2 Updated by Marc-Andre Lafortune over 2 years ago

  • Category set to core
  • Target version set to 2.0.0

Indeed, the documentation does not match the code and there is no test for this.

It was clearly Matz' intention to return the (last) deleted element: https://bugs.ruby-lang.org/projects/ruby-trunk/repository/revisions/18527

This is the most helpful behavior anyways.

I'll note that the documentation is clearly wrong in the second form too.

Unless there is objection, I'll commit the following patch:

diff --git a/array.c b/array.c
index df0a0a4..481eebc 100644
--- a/array.c
+++ b/array.c
@@ -2605,12 +2605,12 @@ rb_ary_keep_if(VALUE ary)

/*
* call-seq:
- * ary.delete(obj) -> obj or nil
- * ary.delete(obj) { block } -> obj or nil
+ * ary.delete(obj) -> element or nil
+ * ary.delete(obj) { block } -> element or result of block
*
* Deletes all items from +self+ that are equal to +obj+.
*
- * If any items are found, returns +obj+, otherwise +nil+ is returned instead.
+ * Returns the last deleted item, or +nil+ if no matching item is found.
*
* If the optional code block is given, the result of the block is returned if
* the item is not found. (To remove +nil+ elements and get an informative
diff --git a/test/ruby/test_array.rb b/test/ruby/test_array.rb
index 8d264d9..6466fc3 100644
--- a/test/ruby/test_array.rb
+++ b/test/ruby/test_array.rb
@@ -598,6 +598,14 @@ class TestArray < Test::Unit::TestCase
a = @cls[*('cab'..'cat').to_a]
assert_equal(99, a.delete('cup') { 99 } )
assert_equal(@cls[*('cab'..'cat').to_a], a)
+
+ o = Object.new
+ def o.==(other); true; end
+ o2 = Object.new
+ def o2.==(other); true; end
+ a = @cls[1, o, o2, 2]
+ assert_equal(o2, a.delete(42))
+ assert_equal([1, 2], a)
end

def test_delete_at

#3 Updated by Yusuke Endoh over 2 years ago

  • Status changed from Open to Assigned
  • Assignee set to Marc-Andre Lafortune

#4 Updated by Charles Nutter over 2 years ago

marcandre (Marc-Andre Lafortune) wrote:

Indeed, the documentation does not match the code and there is no test for this.

It was clearly Matz' intention to return the (last) deleted element: https://bugs.ruby-lang.org/projects/ruby-trunk/repository/revisions/18527

This is the most helpful behavior anyways.

I'll note that the documentation is clearly wrong in the second form too.

Unless there is objection, I'll commit the following patch:

Yes, it appears to have been an explicit behavioral change in the 1.9.1/1.8.7 timeframe that never got reflected in documentation.

#5 Updated by Marc-Andre Lafortune over 2 years ago

  • Status changed from Assigned to Closed

Documentation fixed, thanks for bringing this up.

Also available in: Atom PDF