-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add CSV.parse - Support Headers #15258
base: master
Are you sure you want to change the base?
Add CSV.parse - Support Headers #15258
Conversation
@@ -19,6 +19,16 @@ class CSV::Parser | |||
rows | |||
end | |||
|
|||
def parse_to_h : Array(Hash(String, String)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
realizing that this is not a good place since it parses the remaining rows and that means that every row could be the headers. Didnt notice since i tested only CSV and not the parser itself. Either when using this methode, rewind the parser to the begining or somehow extract the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit dubious about rewinding to get the headers. I think the caller can make sure the CSV parser is where it needs to be before calling to_h.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how I feel about parse_to_h
as a name. It's returning an array, so anything to_h
seems misleading.
Is there a reason not to return Array(CSV::Row)
instead of Array(Hash(String, String))
? CSV::Row
has the hash-like and array-like interface.
h = {} of String => String | ||
row.empty? ? return nil : headers.each_with_index { |header, i| h[header] = row[i] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the early return can avoid any heap allocations
- starting with an empty hash results in
log(n)
allocations, but since we already know how many entries the hash will be, we can allocate that many elements so we only do 1 allocation
h = {} of String => String | |
row.empty? ? return nil : headers.each_with_index { |header, i| h[header] = row[i] } | |
return nil if row.empty? | |
h = Hash(String, String).new(initial_capacity: headers.size) | |
headers.each_with_index { |header, i| h[header] = row[i] } |
Based on issue #15170 requirements and solution recommendations.
closes #15170, if this PR is accepted and merged.