Skip to content

Add Row#each_pair - #33

Merged
kou merged 1 commit into
ruby:masterfrom
zverok:row-each-pair
Apr 2, 2018
Merged

Add Row#each_pair#33
kou merged 1 commit into
ruby:masterfrom
zverok:row-each-pair

Conversation

@zverok

@zverok zverok commented Mar 25, 2018

Copy link
Copy Markdown
Contributor

Reason: Row tries to quack more-or-less like Hash, and Hash has #each_pair. A practical example where it matters:

csv = CSV.parse('Bob,100', headers: %i[name salary])
OpenStruct.new(csv.first)
# NoMethodError: undefined method `each_pair' for #<CSV::Row name:"Bob" salary:"100">

# After this patch:
OpenStruct.new(csv.first)
#=> #<OpenStruct name="Bob", salary="100">

@kou

kou commented Mar 31, 2018

Copy link
Copy Markdown
Member

Do you want to try adding a test?

@zverok

zverok commented Apr 1, 2018

Copy link
Copy Markdown
Contributor Author

Thought about it, but I am not sure what is an appropriate amount of testing. It is just an alias, so do you expect me to copy-paste of tests of #each, or just literally test "the alias, named this, exists"?

@kou

kou commented Apr 1, 2018

Copy link
Copy Markdown
Member

This change isn't so simple.

What is the expected behavior of the following case?

require "csv"
csv = CSV.parse('Bob,100', headers: %i[name salary])
p csv.first.each_pair.to_a
# /tmp/a.rb:3:in `<main>': undefined method `each_pair' for ["Bob", "100"]:Array (NoMethodError)

@zverok

zverok commented Apr 1, 2018

Copy link
Copy Markdown
Contributor Author

What is the expected behavior of the following case?

On my branch:

csv.first.each_pair.to_a
# => [[:name, "Bob"], [:salary, "100"]]

Is there some problem?..

@kou

kou commented Apr 1, 2018

Copy link
Copy Markdown
Member

Ah, sorry. I pasted wrong code:

require "csv"
csv = CSV.parse('Bob,100')
p csv.first.each_pair.to_a 

@kou

kou commented Apr 1, 2018

Copy link
Copy Markdown
Member

Ah. it's not related with this change.
It's my fault.

@kou
kou merged commit 3e77167 into ruby:master Apr 2, 2018
@zverok
zverok deleted the row-each-pair branch May 18, 2018 16:19
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