Crystal: Remove delete_if

[caspiano]

Following the discussion in #9856, Hash#delete_if and Deque#delete_if are duplicates of reject!, and we don't like duplicated functionality 'round these parts.

These methods also have inconsistent interfaces...

• Hash#delete_if returns self
• Deque#delete_if returns Bool

My proposal is to...

• remove Hash#delete_if and Deque#delete_if
• add Deque#reject! to replace Deque#delete_if
• Given the inclusion of Deque#reject!, we should intuitively implement Deque#select!

A draft implementation of this proposal can be found here #9878

[straight-shoota]

It certainly makes sense to standardize this and dropping delete_if in favour of reject! seems the best solution.

There are some subtle differences in the semantics of each of these methods and they need to be taken into consideration:

• Hash#delete_if defers deletion until the iteration. Hash#reject! deletes immediately. Both should equally work (see https://github.com/crystal-lang/crystal/pull/9856#issuecomment-719953816). But there are effects outside of Hash itself. For example, when checking whether an item should be deleted, it might compare it with other items in the hash. If deleted entries still exist, the result may be different as when they're already deleted.
• Hash#delete_if always returns self. Hash#reject! returns nil when no entry was deleted. The implementation is very unreliable, though because it just compares size before and after. This can however be fixed by using an actual counter/flag. I'm not sure if change reporting is that useful. It certainly doesn't make much sense to return nilable self because it prevents proper method chaining. The return type should either be Bool for reporting whether ther was a change (for whatever reason you would need that) or non-nilable self to enable method chaining.
• Deque#delete_if returns Bool indicating whether an item was deleted. Deque#reject! should probably just return self instead.

[bcardiff]

• I agree with leaving reject and getting rid of delete_if.
• But I would leave a deprecation on Hash. It is so widely used I don't want to do a breaking-change without a deprecation cycle. Even if that means keeping it until 2.0.

It's a pity that Hash#reject will not offer a way to determine from its result if an element was removed. Thay is available from in Hash#delete, but on the other side #reject will remove multiple entries. If you need to check if something was removed you can use #size I guess (of course, at some extent, as long as the hash is not shared).

[asterite]

will not offer a way to determine from its result if an element was removed

deleted = false
hash.reject! do |k, v|
  if some_condition
    deleted = true
    true
  else
    false
  end
end

So if you really need that you can do it. There's no need for that method to always take that into account if it's not necessary.