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

Rake::TaskArguments#key? alias of #has_key? #175

Merged
merged 1 commit into from
Dec 6, 2016
Merged

Rake::TaskArguments#key? alias of #has_key? #175

merged 1 commit into from
Dec 6, 2016

Conversation

pda
Copy link
Contributor

@pda pda commented Dec 6, 2016

This PR adds Rake::TaskArguments#key?(key) as an alias of has_key?(key).


Rake::TaskArguments exposes a Hash-like interface including #has_key?(key), but is missing #key?(key). Making this worse, the method_missing behaviour means args.key?(:foo) does not raise NoMethodError, instead treating it like args[:key?] and generally returning nil.

The commonly referenced ruby-style-guide says:

Use Hash#key? instead of Hash#has_key? … As noted here by Matz, the longer forms are considered deprecated.

which links to matz saying:

"has_key" has already been deprecated by "key?"

RuboCop's Style/PreferredHashMethods rule complains about using has_key?:

lib/tasks/feed_consumer.rake:11:60: C: Use Hash#key? instead of Hash#has_key?.
    raise("task requires `interval` argument") unless args.has_key?(:interval)
                                                           ^^^^^^^^

However accepting its solution of using #key? causes silent failure via method_missing as described above.

@hsbt
Copy link
Member

hsbt commented Dec 6, 2016

test fail with JRuby-9.1.5.0 is not related your pull request. I approved your patch. I will merge later.

@hsbt hsbt merged commit 1b62cbb into ruby:master Dec 6, 2016
@pda
Copy link
Contributor Author

pda commented Dec 8, 2016

Thanks @hsbt, and thanks also for releasing v12.0.0 👍

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.

2 participants