8000 Fix TaskArguments#deconstruct_keys with keys = nil by nevans · Pull Request #635 · ruby/rake · GitHub
[go: up one dir, main page]

Skip to content

Fix TaskArguments#deconstruct_keys with keys = nil #635

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nevans
Copy link
@nevans nevans commented Jun 2, 2025

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     # => 1
  rest  # => {b: 2, c: 3}

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

  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)

`#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 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 }
Copy link
Contributor
@rgarner rgarner Jun 2, 2025

Choose a reason for hiding this comment

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

Suggested change
assert_equal ta.deconstruct_keys(nil), { a: 1, b: 2, c: 3 }
assert_equal ta.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 😉)

Copy link
Author
@nevans nevans Jun 4, 2025

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>
Sign up for free to 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.

2 participants
0