Project

General

Profile

Feature #16697

Hash.ruby2_keywords_hash?(value) should support any object

Added by Eregon (Benoit Daloze) 10 months ago. Updated about 1 month ago.

Status:
Rejected
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:97554]

Description

But currently it raises, which makes it needlessly inconvenient to use:

> Hash.ruby2_keywords_hash?({})
=> false
> Hash.ruby2_keywords_hash?("hello")
Traceback (most recent call last):
        5: from /home/eregon/.rubies/ruby-trunk/bin/irb:23:in `<main>'
        4: from /home/eregon/.rubies/ruby-trunk/bin/irb:23:in `load'
        3: from /home/eregon/prefix/ruby-trunk/lib/ruby/gems/2.8.0/gems/irb-1.2.3/exe/irb:11:in `<top (required)>'
        2: from (irb):4
        1: from (irb):4:in `ruby2_keywords_hash?'
TypeError (wrong argument type String (expected Hash))

See https://github.com/ruby/ruby/pull/2966/files#r394741112 for a motivating example.

I'd like to suggest backporting this to 2.7 too.

#1

Updated by hsbt (Hiroshi SHIBATA) 4 months ago

  • Target version changed from 36 to 3.0

Updated by jbeschi (jacopo beschi) about 2 months ago

I'd like to contribute here but I'm new to this so please forgive me if this is a stupid question: we basically just need to change ruby2_keywords_hash? to return false if the argument is not an Hash instead of raising a TypeError ?

Updated by Eregon (Benoit Daloze) about 2 months ago

I think we need to decide if this change is accepted, I'll add it to the dev meeting agenda.

Indeed, the concrete change would just be that, feel free to give it a try.

#4

Updated by Eregon (Benoit Daloze) about 2 months ago

  • Backport deleted (2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN)
  • ruby -v deleted (ruby 2.8.0dev (2020-02-28T14:32:56Z master f7be85a2b7) [x86_64-linux])
  • Tracker changed from Bug to Feature

Updated by jbeschi (jacopo beschi) about 2 months ago

Thanks! I'll give it a try :-)

Updated by mame (Yusuke Endoh) about 2 months ago

I'm a bit afraid if changing the behavior would bring confusion rather than trivial usability. It is easy to workaround the issue.

Anyway, if it is changed, we need to backport the patch to ruby_2_7. If ruby_2_7 and master are inconsistent, it is more confusing. nagachika (Tomoyuki Chikanaga) -san, will you backport it if it is accepted?

Updated by Dan0042 (Daniel DeLorme) about 2 months ago

Maybe return nil if the object is not a Hash?

Updated by jbeschi (jacopo beschi) about 2 months ago

Honestly, I don't think returning nil would be an improvement: using nil as a type check looks a bit hacky to me.

Anyways I'd wait for the devs meeting outcome and see if we can move this forward.

Updated by mame (Yusuke Endoh) about 2 months ago

jbeschi (jacopo beschi) wrote in #note-9:

Anyways I'd wait for the devs meeting outcome and see if we can move this forward.

My comment #7 is the meeting outcome. Some committers including matz discussed this issue, but we were not sure if the change is really needed. Even if we change it in Ruby 2.7.3 and 3.0.0, people cannot depend on the new behavior if they care users of Ruby 2.7.2. Also, while it is trivial to workaround the issue, changing the behavior may bring new confusion.

Anyway, this issue must be handled not only in master but also in ruby 2.7 branch. nagachika (Tomoyuki Chikanaga) , who is Ruby 2.7 branch maintainer, didn't attend the meeting. So we first ask nagachika (Tomoyuki Chikanaga) 's opinion.

Updated by jbeschi (jacopo beschi) about 1 month ago

mame (Yusuke Endoh) Thanks for clarifying! Ok let's see :)

Updated by mame (Yusuke Endoh) about 1 month ago

  • Target version deleted (3.0)
  • Status changed from Open to Feedback

I don't think we will be able to decide this issue by Ruby 3.0.0. But changing it after Ruby 3.0.0 seems more confusing. I vote for WONTFIX if there is no strong reason.

Updated by Eregon (Benoit Daloze) about 1 month ago

  • Status changed from Feedback to Closed

OK, let's reject this then.

#14

Updated by Eregon (Benoit Daloze) about 1 month ago

  • Status changed from Closed to Rejected

Also available in: Atom PDF