Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use private_method_defined? instead of respond_to? #8

Merged
merged 1 commit into from
Jan 7, 2021

Conversation

mame
Copy link
Member

@mame mame commented Jan 7, 2021

Module.respond_to?(:ruby2_keywords, true) does NOT check if
Module#ruby2_keywords is available. It worked well because there is
toplevel ruby2_keywords method, but using private_method_defined? is
better, I think.

Also, this fixes a syntactic error.

`Module.respond_to?(:ruby2_keywords, true)` does NOT check if
`Module#ruby2_keywords` is available. It worked well because there is
toplevel `ruby2_keywords` method, but using `private_method_defined?` is
better, I think.

Also, this fixes a syntactic error.
@mame mame requested a review from nobu January 7, 2021 08:42
@nobu nobu merged commit 52c15f0 into ruby:master Jan 7, 2021
@@ -1,5 +1,5 @@
class Module
unless respond_to?(:ruby2_keywords, true)
unless private_method_defined?(:ruby2_keywords, true)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke this gem on Ruby 2.5:

ArgumentError: wrong number of arguments (given 2, expected 1)
/usr/local/bundle/gems/ruby2_keywords-0.0.3/lib/ruby2_keywords.rb:2:in `private_method_defined?'

@@ -1,5 +1,5 @@
class Module
unless respond_to?(:ruby2_keywords, true)
unless private_method_defined?(:ruby2_keywords, true)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke this gem on Ruby 2.5:

ArgumentError: wrong number of arguments (given 2, expected 1)
/usr/local/bundle/gems/ruby2_keywords-0.0.3/lib/ruby2_keywords.rb:2:in `private_method_defined?'

@eregon
Copy link
Member

eregon commented Jan 19, 2021

It also broke TruffleRuby FWIW (notably, gem install sinatra fails now).
Of course we'll fix TruffleRuby master to handle 2-args private_method_defined?, but it won't fix existing releases, notably 21.0.0 coming out today.

Module.respond_to?(:ruby2_keywords, true) does NOT check if
Module#ruby2_keywords is available.

I believe that's incorrect.

respond_to?(:ruby2_keywords, true) is what everyone uses, and AFAIK it's correct, it finds Module#ruby2_keywords:

ruby -ve 'class Module; p method(:ruby2_keywords).owner; end'
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-linux]
Module

i.e., we check if self == #<Class:Module>, which is an instance of Class < Module has a method ruby2_keywords, and yes, there is Module#ruby2_keywords.

Please revert and re-release.

proc {|*a, **h| h}.call(
ensure
$VERBOSE = verbose
unless method_defined?(:ruby2_keywords_hash?)
Copy link
Member

@eregon eregon Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an incorrect check BTW, there is Hash.ruby2_keywords_hash? and no Hash#ruby2_keywords_hash?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nobu fixed it in 51c47c0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants