Skip to content

Fix TaskArguments#deconstruct_keys with keys = nil - #635

Merged
hsbt merged 2 commits into
ruby:masterfrom
nevans:deconstruct_keys-for-nil-keys
Oct 29, 2025
Merged

Fix TaskArguments#deconstruct_keys with keys = nil#635
hsbt merged 2 commits into
ruby:masterfrom
nevans:deconstruct_keys-for-nil-keys

Conversation

@nevans

Copy link
Copy Markdown
Contributor

This fixes a bug in the initial implementation (#515):

#deconstruct_keys should handle keys == nil, or **rest will be broken.

For example, the normal behavior looks like this:

{a: 1,b: 2,c: 3}=>{a:, **rest}a# => 1rest# => {b: 2, c: 3}

But without handling keys == nil, we'll get this:

classExampledefinitialize(hash)=@hash=hashdefdeconstruct_keys(keys)=@hash.slice(*keys)endExample.new({a: 1,b: 2,c: 3})=>{a:, **rest}# !> "#{inspect}: key not found: :a" (NoMatchingPatternKeyError)

`#deconstruct_keys` should handle `keys == nil`, or `**rest` will be broken.
For example, the normal behavior looks like this:
```ruby
{a: 1, b: 2, c: 3} => {a:, **rest}
a # => 1
rest # => {b: 2, c: 3}
```
But without handling `keys == nil`, we'll get this:
```ruby
class Example
def initialize(hash) = @hash = hash
def deconstruct_keys(keys) = @hash.slice(*keys)
end
Example.new({a: 1, b: 2, c: 3}) => {a:, **rest}
# !> "#{inspect}: key not found: :a" (NoMatchingPatternKeyError)
```
Comment threadtest/test_rake_task_arguments.rb Outdated
Comment on lines 61 to 62
assert_equal ta.deconstruct_keys(nil), { a: 1, b: 2, c: 3 }
assert_equal ta.deconstruct_keys([:a, :b]), { a: 1, b: 2 }

@rgarnerrgarnerJun 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert_equalta.deconstruct_keys(nil),{a: 1,b: 2,c: 3}
assert_equalta.deconstruct_keys([:a,:b]),{a: 1,b: 2}
assert_equal({a: 1,b: 2,c: 3},ta.deconstruct_keys(nil))
assert_equal({a: 1,b: 2},ta.deconstruct_keys([:a,:b]))

Being an RSpec devotee, I spoke with an accent when I added this test and had the assert_equal(expected, actual) order backwards. It'd only affect the failure message but I'd hate for someone plugging a hole to copy my bad accent...

(I went to fix it but then noticed you'd opened this PR! Too quick for me 😉)

@nevansnevansJun 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm also an rspec guy, so I get the order of these backwards pretty often myself. I certainly didn't notice they were in the wrong order here! 😉 Anyway, I pushed an update with them in the correct order. Thanks. 🙂

Co-authored-by: Russell Garner <rgarner@zephyros-systems.co.uk>
@hsbt
hsbt merged commit ec12ac9 into ruby:masterOct 29, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@nevans@hsbt@rgarner