Skip to content

Conversation

kddnewton
Copy link

No description provided.

lib/csv/row.rb Outdated
# Returns the new \Hash suitable for pattern matching containing only the
# keys specified as an argument.
def deconstruct_keys(keys)
keys.to_h { |key| [key, self[key]] }

Choose a reason for hiding this comment

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

The keys, as supplied, will be Symbols. This can present an issue as CSV rows will more than likely be String keyed:

require 'csv'
# => true

csv = CSV.parse <<~RUBY, headers: true
  a,b,c
  1,2,3
  2,3,4
  4,5,6
RUBY
# => #<CSV::Table mode:col_or_row row_count:4>

csv.headers
# => ["a", "b", "c"]

csv.to_a
# => [["a", "b", "c"], ["1", "2", "3"], ["2", "3", "4"], ["4", "5", "6"]]

csv.first.class
# => CSV::Row

csv.first.fields
# => ["1", "2", "3"]
csv.first.headers
# => ["a", "b", "c"]

If you want to support this you would want to instead transform the keys to String, but this may be controversial as it conflates the two.

Choose a reason for hiding this comment

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

This could be resolved one of a few ways.

In the first, retrieval of values would be done versus the String equivalent of the value:

def deconstruct_keys(keys)
  keys.to_h { |k| [k, self[k.to_s]] }
end

This assumes, however commonly it may be, that all keys are String values. This could be resolved via:

def deconstruct_keys(keys)
  first_header = headers.first
  case first_header
  when String then keys.to_h { |k| [k, self[k.to_s]] }
  when Symbol then keys.to_h { |k| [k, self[k]] }
  else {}
  end
end

...though this is far more verbose. I do not think that Symbol keys are commonly used in the context of CSV, so this concern might be ignored safely, but I leave that up to the core group of CSV to decide the risk.

Copy link
Author

Choose a reason for hiding this comment

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

I think I'd rather leave it as is for now. You can always pass header_converters: :symbol to the parse function, which would make this work as expected.

I think I would be really surprised if in my matching I specified symbols and it matched against strings.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Sorry for not responding this.
Could you add support for in **rest case?


def test_pattern_matching_hash
case CSV::Row.new(%i{A B C}, [1, 2, 3])
in B: b, C: c
Copy link
Member

Choose a reason for hiding this comment

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

Could you add one more test case for in B:, **rest?

Copy link
Author

Choose a reason for hiding this comment

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

Updated!

@kou kou merged commit a01c8f2 into ruby:master Jun 5, 2021
@kou
Copy link
Member

kou commented Jun 5, 2021

Thanks!

@kddnewton kddnewton deleted the csv-pattern-matching branch June 7, 2021 01:49
sanfrecce-osaka added a commit to sanfrecce-osaka/doctree that referenced this pull request Jun 11, 2024
cf. https://docs.ruby-lang.org/en/3.3/CSV/Row.html#method-i-deconstruct

rdoc にはサンプルコードがなかったためオリジナルのものを追加している
また ruby/csv#207 (comment) の挙動は記載しておくべきと考えたため説明を追加している

追加は csv v3.2.0 からでこのバージョンが標準添付されるようになったのは v3.1.0 から

cf. ruby/csv@a01c8f2
cf. https://github.com/ruby/ruby/blob/v3_3_2/doc/NEWS/NEWS-3.1.0.md
sanfrecce-osaka added a commit to sanfrecce-osaka/doctree that referenced this pull request Jun 11, 2024
cf. https://docs.ruby-lang.org/en/3.3/CSV/Row.html#method-i-deconstruct

rdoc にはサンプルコードがなかったためオリジナルのものを追加している
また ruby/csv#207 (comment) の挙動は記載しておくべきと考えたため説明を追加している

追加は csv v3.2.0 からでこのバージョンが標準添付されるようになったのは v3.1.0 から

cf. ruby/csv@a01c8f2
cf. https://github.com/ruby/ruby/blob/v3_3_2/doc/NEWS/NEWS-3.1.0.md
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.

3 participants