Conversation
| new_key, new_value = if block_given? | ||
| yield(key, value) | ||
| else | ||
| [key, value] |
There was a problem hiding this comment.
We don't want to create a needless temporary Array.
Could you use
if block_given?
each do |key, value|
key, value = yield(key, value)
# ...
end
else
each do |key, value|
# ...
end
endor
each do |key, value|
key, value = yield(key, value) if block_given?
# ...
end?
There was a problem hiding this comment.
Thanks, that's a good one
| row2 = CSV::Row.new(%w{A B C}, [1, 2, 3]) | ||
| hash2 = row2.to_hash { |k, v| [k, v ** 2] } | ||
| assert_equal({"A" => 1, "B" => 4, "C" => 9}, hash2) |
| each do |key, value| | ||
| key, value = yield(key, value) if block_given? |
There was a problem hiding this comment.
Ah, sorry. We need to call self[key]:
| each do |key, value| | |
| key, value = yield(key, value) if block_given? | |
| each do |key, _value| | |
| value = self[key] | |
| key, value = yield(key, value) if block_given? |
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
There was a problem hiding this comment.
Pull request overview
Adds support for passing a block to CSV::Row#to_h / #to_hash, aligning usage with Ruby’s to_h { ... } pattern and avoiding the need for row.entries.to_h { ... }.
Changes:
- Add a new test covering
CSV::Row#to_hashwith a block. - Update
CSV::Row#to_hto yield each key/value pair to an optional block before insertion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/csv/test_row.rb | Adds a test for block form of to_hash. |
| lib/csv/row.rb | Implements yielding to an optional block in to_h during hash construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| each do |key, _value| | ||
| hash[key] = self[key] unless hash.key?(key) | ||
| value = self[key] | ||
| key, value = yield(key, value) if block_given? | ||
|
|
||
| hash[key] = value unless hash.key?(key) |
| each do |key, _value| | ||
| hash[key] = self[key] unless hash.key?(key) | ||
| value = self[key] | ||
| key, value = yield(key, value) if block_given? |
| def to_h | ||
| hash = {} | ||
| each do |key, _value| | ||
| hash[key] = self[key] unless hash.key?(key) | ||
| value = self[key] | ||
| key, value = yield(key, value) if block_given? | ||
|
|
||
| hash[key] = value unless hash.key?(key) |
| row = CSV::Row.new(%w{A B C}, [1, 2, 3]) | ||
| hash = row.to_hash { |k, v| [k, v ** 2] } |
| def to_h | ||
| hash = {} | ||
| each do |key, _value| | ||
| hash[key] = self[key] unless hash.key?(key) | ||
| value = self[key] | ||
| key, value = yield(key, value) if block_given? |
| hash = {} | ||
| each do |key, _value| | ||
| hash[key] = self[key] unless hash.key?(key) | ||
| next if hash.key?(key) |
There was a problem hiding this comment.
key may be changed by yield(key, value). So we can't do this if block_given?.
How about using separated each?
if block_given?
each do |key, _value|
key, value = yield(key, self[key])
hash[key] = value unless hash.key?(key)
end
else
each do |key, _value|
hash[key] = self[key] unless hash.key?(key)
end
endThere was a problem hiding this comment.
Pull request overview
Adds block support to CSV::Row#to_h (and therefore #to_hash) to align with Ruby’s to_h { ... } conversion pattern, so callers don’t need to go through row.entries.to_h { ... }.
Changes:
- Extend
CSV::Row#to_hto accept an optional block that transforms each (key, value) pair before insertion. - Document the new block form in
CSV::Row#to_h’s RDoc. - Add a unit test exercising
to_hashwith a block.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/csv/test_row.rb | Adds coverage for CSV::Row#to_hash accepting a block. |
| lib/csv/row.rb | Implements and documents block support for CSV::Row#to_h/#to_hash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| each do |key, _value| | ||
| hash[key] = self[key] unless hash.key?(key) | ||
| next if hash.key?(key) | ||
|
|
||
| value = self[key] | ||
| key, value = yield(key, value) if block_given? | ||
|
|
||
| hash[key] = value |
| next if hash.key?(key) | ||
|
|
||
| value = self[key] | ||
| key, value = yield(key, value) if block_given? |
| row = CSV::Row.new(%w{A A B C}, [1, 2, 2, 3]) | ||
| hash = row.to_hash { |k, v| [k, v**2] } | ||
| assert_equal({"A" => 1, "B" => 4, "C" => 9}, hash) |
There was a problem hiding this comment.
Pull request overview
This PR adds support for passing a block to CSV::Row#to_h/#to_hash, aligning the API shape with Ruby’s to_h { ... } usage while keeping CSV::Row#to_h as the primary conversion entrypoint.
Changes:
- Add a new test covering
CSV::Row#to_hashwith a block and basic error cases for invalid block return values. - Extend
CSV::Row#to_hto accept an optional block and transform(key, value)pairs via the block result. - Update the
to_hmethod documentation to include the block form and an example.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
test/csv/test_row.rb |
Adds coverage for to_hash when a block is provided, including error handling expectations. |
lib/csv/row.rb |
Implements block support in CSV::Row#to_h and updates the method docs accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise TypeError, "wrong element type #{result.class} (expected array)" unless result.is_a?(Array) | ||
| raise ArgumentError, "wrong array length (expected 2, was #{result.size})" unless result.size == 2 | ||
|
|
||
| key, value = result |
| if block_given? | ||
| each do |key, _value| | ||
| result = yield(key, self[key]) | ||
| raise TypeError, "wrong element type #{result.class} (expected array)" unless result.is_a?(Array) | ||
| raise ArgumentError, "wrong array length (expected 2, was #{result.size})" unless result.size == 2 | ||
|
|
||
| key, value = result | ||
| hash[key] = value unless hash.key?(key) | ||
| end |
| new_keys_map = {"A" => "A", "B" => "B", "C" => "B"} | ||
| hash = row.to_hash { |k, v| [new_keys_map[k], v**2] } | ||
| assert_equal({"A" => 1, "B" => 4}, hash) | ||
| hash.keys.each_with_index do |string_key, h| |
| raise TypeError, "wrong element type #{result.class} (expected array)" unless result.is_a?(Array) | ||
| raise ArgumentError, "wrong array length (expected 2, was #{result.size})" unless result.size == 2 | ||
|
|
||
| key, value = result |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # source = "Name,Value\nfoo,1\nbar,2\nbaz,3\n" | ||
| # table = CSV.parse(source, headers: true) | ||
| # row = table[0] | ||
| # row.to_h { |key, value| [key, value.to_i * 2] } # => {"Name"=>"foo", "Value"=>2} |
| result = Array.try_convert(yield(key, self[key])) | ||
| raise TypeError, "wrong element type #{result.class} (expected array)" if result.nil? | ||
| raise ArgumentError, "wrong array length (expected 2, was #{result.size})" unless result.size == 2 | ||
|
|
||
| key, value = result |
|
|
||
| if block_given? | ||
| each do |key, _value| | ||
| result = Array.try_convert(yield(key, self[key])) |
| each do |key, _value| | ||
| hash[key] = self[key] unless hash.key?(key) | ||
| end |
| def test_to_hash_with_block | ||
| row = CSV::Row.new(%w{A A B C}, [1, 2, 2, 3]) | ||
| new_keys_map = {"A" => "A", "B" => "B", "C" => "B"} | ||
| hash = row.to_hash { |k, v| [new_keys_map[k], v**2] } | ||
| assert_equal({"A" => 1, "B" => 4}, hash) |
| key.freeze if key.is_a?(String) && !key.frozen? | ||
| hash[key] = value unless hash.key?(key) |
There was a problem hiding this comment.
Could you avoid needless freeze check?
| key.freeze if key.is_a?(String) && !key.frozen? | |
| hash[key] = value unless hash.key?(key) | |
| next if hash.key?(key) | |
| key.freeze if key.is_a?(String) && !key.frozen? | |
| hash[key] = value |
There was a problem hiding this comment.
Oh sorry I didn't see code suggestion and thought it's about removing !key.frozen?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if block_given? | ||
| each do |key, _value| | ||
| result = yield(key, self[key]) | ||
| result_array = Array.try_convert(result) | ||
| raise TypeError, "wrong element type #{result.class} (expected array)" if result_array.nil? | ||
| raise ArgumentError, "wrong array length (expected 2, was #{result_array.size})" unless result_array.size == 2 | ||
|
|
||
| key, value = result_array | ||
| key.freeze if key.is_a?(String) | ||
| hash[key] = value unless hash.key?(key) | ||
| end |
| def test_to_hash_with_block | ||
| row = CSV::Row.new(%w{A A B C}, [1, 2, 2, 3]) | ||
| new_keys_map = {"A" => "A", "B" => "B", "C" => "B"} | ||
| hash = row.to_hash { |k, v| [new_keys_map[k], v**2] } | ||
| assert_equal({"A" => 1, "B" => 4}, hash) | ||
| hash.each_key do |string_key| | ||
| assert_predicate(string_key, :frozen?) | ||
| end | ||
|
|
||
| assert_raise TypeError do | ||
| row.to_hash { "foo" } | ||
| end | ||
|
|
||
| assert_raise ArgumentError do | ||
| row.to_hash { [1] } | ||
| end | ||
| end |
| new_keys_map = {"A" => "A", "B" => "B", "C" => "B"} | ||
| hash = row.to_hash { |k, v| [new_keys_map[k], v**2] } | ||
| assert_equal({"A" => 1, "B" => 4}, hash) | ||
| hash.each_key do |string_key| | ||
| hash2 = row.to_hash { |k, v| [new_keys_map[k], v**2] } | ||
| assert_equal({"A" => 1, "B" => 4}, hash2) | ||
| hash2.each_key do |string_key| | ||
| assert_predicate(string_key, :frozen?) | ||
| end |
There was a problem hiding this comment.
Could you split this duplicated key case to a separated test?
We can merge this after it.
| key.freeze if key.is_a?(String) && !key.frozen? | ||
| hash[key] = value unless hash.key?(key) |
|
Thanks. Do you want to use this feature soon? (Should we release a new version soon?) |
|
@kou No rush, I have a patch in project Thanks |
It will be consistent with Ruby's method, otherwise it has to be like
row.entries.to_h { ... }