Skip to content

Conversation

@BurdetteLamar
Copy link
Member

New Recipes page is oriented to what the user wants to do (instead of what CSV can do). User finds in Contents the relevant task, follows link to get example working code.

Fulsome discussion welcome.


Instance method CSV#read can read a file all at once:
string = "foo,0\nbar,1\nbaz,2\n"
CSV.new(string).read # => [["foo", "0"], ["bar", "1"], ["baz", "2"]]
Copy link
Member

Choose a reason for hiding this comment

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

CSV.read(path)?

Comment on lines 123 to 124
File.open(path, headers: true) do |file|
CSV.parse(file)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
File.open(path, headers: true) do |file|
CSV.parse(file)
File.open(path) do |file|
CSV.parse(file, headers: true)

We also need to update output.


in_string = "Name,Value\nfoo,0\nbar,1\nbaz,2\n"
path = 't.csv'
File.open(path, 'w', headers: true) do |out_io|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
File.open(path, 'w', headers: true) do |out_io|
File.open(path, 'w') do |out_io|

Comment on lines 278 to 279
File.open(path) do |file|
CSV.filter(in_string, out_string) do |row|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
File.open(path) do |file|
CSV.filter(in_string, out_string) do |row|
File.open(path) do |in_io|
CSV.filter(in_io, out_string) do |row|

@BurdetteLamar
Copy link
Member Author

Thanks, @kou. Did you like the idea of recipes? If so, there are many more possibilities.

@kou
Copy link
Member

kou commented Sep 17, 2020

Yes! I like this.

@BurdetteLamar BurdetteLamar requested a review from kou September 17, 2020 16:58
Copy link
Contributor

@esparta esparta left a comment

Choose a reason for hiding this comment

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

minimal typo & phrase suggestion

==== Parse from \String Without Headers

\Class method CSV.parse can read a source \String all at once,
and so may have memory resource implications:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and so may have memory resource implications:
be aware this <procedure> may have memory resource implications:

Not sure about adding procedure or something different.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm leaving this as is. CSV.parse is the subject in both clauses, and so the sense, as I see it, is "CSV.parse may have memory resource implications."

@BurdetteLamar
Copy link
Member Author

@kou, this may be ready to merge.

@kou kou marked this pull request as ready for review September 17, 2020 21:04
@BurdetteLamar BurdetteLamar requested review from kou and removed request for kou September 17, 2020 21:48
@kou kou merged commit 01ffd0d into ruby:master Sep 17, 2020
@kou
Copy link
Member

kou commented Sep 17, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants