Allow CSV.open with StringIO argument by MarcPer · Pull Request #302 · ruby/csv

MarcPer · 2024-05-04T11:45:49Z

kou · 2024-05-24T08:41:21Z

+ pos = filename_or_io.pos
+ f = StringIO.new(filename_or_io.read)
+ filename_or_io.seek(pos)


Can we use StringIO#string here?
Suggested change
pos=filename_or_io.pos
f=StringIO.new(filename_or_io.read)
filename_or_io.seek(pos)
f=StringIO.new(filename_or_io.string)

We'd then run into an issue similar to #98, which I confirmed with another test:
deftest_foreach_stringio_with_boms=StringIO.news << "\ufeff"# BOMs << @datas.rewinds.read(3)rows=CSV.foreach(s,col_sep: "\t",row_sep: "\r\n").to_aassert_equal(@rows,rows)end

I guess the most relevant counter-argument is, CSV.new also uses read, so using read here is also a matter of consistency.

Good point. Could you also add a test for the BOM case?
How about using StringIO#set_encoding_by_bom for it?
f = StringIO.new(filename_or_io.string) unless f.set_encoding_by_bom f.set_encoding(filename_or_io.external_encoding) end
CSV.new uses already opened IO. So it must not ignore the current position. So we must use read.
But CSV.open opens a new IO. So it must use all contents instead of contents after the current position.
Could you check the current behavior of CSV.open with sought real IO?

But CSV.open opens a new IO. So it must use all contents instead of contents after the current position.
You're right, CSV.open reads the whole file content. I changed open again to use string instead of read.
Could you also add a test for the BOM case?
Sure thing. I added it, and together with using the **file_opts in the new StringIO, it's handled correctly.
How about using StringIO#set_encoding_by_bom for it?
I couldn't see any effect of doing it. It seems like including the file_opts is enough. Or is there a case I'm missing?

I couldn't see any effect of doing it.
Ah, b706d91 is a trick for it.

Is there anything you still feel is missing, after I've added file_opts with the trick from b706d91 ?

kou · 2024-06-04T21:23:49Z

Could you rebase on main? Our CI on master broken... and I've fixed it now.

MarcPer · 2024-06-05T06:23:14Z

Could you rebase on main? Our CI on master broken... and I've fixed it now.

Thank you, I've rebase it now.

kou · 2024-06-06T01:31:11Z

Thanks. But sorry... The master CI was broken... I've fixed it. Could you rebase on master again?

MarcPer · 2024-06-06T14:37:39Z

Thanks. But sorry... The master CI was broken... I've fixed it. Could you rebase on master again?

No problem, I rebased it now.

MarcPer · 2024-06-13T20:02:08Z

I'm assuming the test failures are related to https://bugs.ruby-lang.org/issues/20526, right?

kou · 2024-06-14T00:32:34Z

Hmm. It should be fixed by 47bf76f .
Could you rebase again?

MarcPer · 2024-06-14T09:57:24Z

Rebased it, and removed a now unneeded check when closing the IO within CSV#open.

kou · 2024-06-16T21:16:06Z

Ah, the failed test is an added test.

Does this work?

diff --git a/lib/csv.rb b/lib/csv.rb
index 7511ced..d2d1c57 100644
--- a/lib/csv.rb+++ b/lib/csv.rb@@ -1591,7 +1591,10 @@ class CSV
# wrap a File opened with the remaining +args+ with no newline
# decorator
file_opts = {}
- may_enable_bom_deletection_automatically(mode, options, file_opts)+ may_enable_bom_deletection_automatically(filename_or_io,+ mode,+ options,+ file_opts)
file_opts.merge!(options)
unless file_opts.key?(:newline)
file_opts[:universal_newline] ||= false
@@ -1900,10 +1903,15 @@ class CSV
private_constant :ON_WINDOWS
private
- def may_enable_bom_deletection_automatically(mode, options, file_opts)- # "bom|utf-8" may be buggy on Windows:- # https://bugs.ruby-lang.org/issues/20526- return if ON_WINDOWS+ def may_enable_bom_deletection_automatically(filename_or_io,+ mode,+ options,+ file_opts)+ unless filename_or_io.is_a?(StringIO)+ # "bom|utf-8" may be buggy on Windows:+ # https://bugs.ruby-lang.org/issues/20526+ return if ON_WINDOWS+ end
return unless Encoding.default_external == Encoding::UTF_8
return if options.key?(:encoding)
return if options.key?(:external_encoding)

MarcPer · 2024-08-01T15:21:25Z

Sorry, I was away for a while. I saw that support to StringIOwas dropped for Ruby 2.6 and earlier, before handling of BOM encoding was added. Given that, would it be okay to skip the new tests for StringIO if RUBY_VERSION < "2.7"? Otherwise, the CSV gem might have to deal itself with detection and stripping of BOM.

…nstead of reusing it

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

This check had been added before we started copying the StringIO argument passed to `#open`, as part of instantiating CSV. Right now we can call `csv.close` even if the underlying IO is a StringIO, without any unexpected side effect.

kou · 2024-08-02T22:44:51Z

+ if RUBY_VERSION >= "2.7"
+ # Support to StringIO was dropped for Ruby 2.6 and earlier:
+ # https://github.com/ruby/stringio/pull/47
+ def test_foreach_stringio
+ string_io = StringIO.new(@data)
+ rows = CSV.foreach(string_io, col_sep: "\t", row_sep: "\r\n").to_a
+ assert_equal(@rows, rows)
+ end
+
+ def test_foreach_stringio_with_bom
+ string_io = StringIO.new("\ufeff#{@data}") # U+FEFF ZERO WIDTH NO-BREAK SPACE
+ rows = CSV.foreach(string_io, col_sep: "\t", row_sep: "\r\n").to_a
+ assert_equal(@rows, rows)
+ end
+ end


Does test_foreach_stringio work with Ruby 2.7? If so, could you omit only test_foreach_stringio_with_bom?
Suggested change
ifRUBY_VERSION >= "2.7"
# Support to StringIO was dropped for Ruby 2.6 and earlier:
# https://github.com/ruby/stringio/pull/47
deftest_foreach_stringio
string_io=StringIO.new(@data)
rows=CSV.foreach(string_io,col_sep: "\t",row_sep: "\r\n").to_a
assert_equal(@rows,rows)
end
deftest_foreach_stringio_with_bom
string_io=StringIO.new("\ufeff#{@data}")# U+FEFF ZERO WIDTH NO-BREAK SPACE
rows=CSV.foreach(string_io,col_sep: "\t",row_sep: "\r\n").to_a
assert_equal(@rows,rows)
end
end
deftest_foreach_stringio
string_io=StringIO.new(@data)
rows=CSV.foreach(string_io,col_sep: "\t",row_sep: "\r\n").to_a
assert_equal(@rows,rows)
end
deftest_foreach_stringio_with_bom
ifRUBY_VERSION < "2.7"
# Support to StringIO was dropped for Ruby 2.6 and earlier without BOM support:
# https://github.com/ruby/stringio/pull/47
omit("StringIO's BOM support isn't available with Ruby < 2.7")
end
string_io=StringIO.new("\ufeff#{@data}")# U+FEFF ZERO WIDTH NO-BREAK SPACE
rows=CSV.foreach(string_io,col_sep: "\t",row_sep: "\r\n").to_a
assert_equal(@rows,rows)
end

It does if I also initialize StringIO without additional options. I just pushed another commit like that. It's unfortunate having to run an additional RUBY_VERSION check at runtime, but at least that's only when calling open.

kou · 2024-08-03T22:33:53Z

+ f = if RUBY_VERSION >= "2.7"
+ StringIO.new(filename_or_io.string, **file_opts)
+ else
+ StringIO.new(filename_or_io.string)
+ end


Can we always use an empty file_opts for Ruby 2.7 or earlier instead?
diff --git a/lib/csv.rb b/lib/csv.rb index 5f75894..6f94bd4 100644 --- a/lib/csv.rb+++ b/lib/csv.rb@@ -1605,11 +1605,7 @@ class CSV options.delete_if {|k, _| /newline\z/.match?(k)} if filename_or_io.is_a?(StringIO) - f = if RUBY_VERSION >= "2.7"- StringIO.new(filename_or_io.string, **file_opts)- else- StringIO.new(filename_or_io.string)- end+ f = StringIO.new(filename_or_io.string, **file_opts) else begin f = File.open(filename_or_io, mode, **file_opts) @@ -1911,7 +1907,11 @@ class CSV mode, options, file_opts) - unless filename_or_io.is_a?(StringIO)+ if filename_or_io.is_a?(StringIO)+ # Support to StringIO was dropped for Ruby 2.6 and earlier without BOM support:+ # https://github.com/ruby/stringio/pull/47+ return if RUBY_VERSION < "3.0"+ else # "bom|utf-8" may be buggy on Windows: # https://bugs.ruby-lang.org/issues/20526 return if ON_WINDOWS

This doesn't work because CSV#open still adds keys to file_opts here:
csv/lib/csv.rb
Lines 1586 to 1589 in bb93c28
file_opts.merge!(options)
unlessfile_opts.key?(:newline)
file_opts[:universal_newline] ||= false
end
For Ruby < 2.7, if file_opts is not empty, we get TypeError: no implicit conversion of Hash into String.
I like that something fails if options is given to CSV#open when it's not supported, as opposed to simply ignoring it, the way I did it, but then the error should be ideally more clear than what the old StringIO returns. I can go that route, by rescuing the TypeError, and raising another exception with a clearer message. In any case, the file_opts[:universal_newline] ||= false would still have to be removed for < 2.7.

This doesn't work because CSV#open still adds keys to file_opts here:
csv/lib/csv.rb
Lines 1586 to 1589 in bb93c28
file_opts.merge!(options)
unlessfile_opts.key?(:newline)
file_opts[:universal_newline] ||= false
end
Oh, sorry.
For Ruby < 2.7, if file_opts is not empty, we get TypeError: no implicit conversion of Hash into String.
I like that something fails if options is given to CSV#open when it's not supported, as opposed to simply ignoring it, the way I did it, but then the error should be ideally more clear than what the old StringIO returns. I can go that route, by rescuing the TypeError, and raising another exception with a clearer message.
I agree with you but can we use raise ... unless file_opts.empty? for it instead of rescuing the TypeError?
In any case, the file_opts[:universal_newline] ||= false would still have to be removed for < 2.7.
I agree with you.

It turns out passing an empty file_opts doesn't work either. I've noticed this quirk from the interpreter in Ruby 2.6:
StringIO.new("bla", **{})# => #<StringIO:0x0000563108bb88b8>opts={}StringIO.new("bla", **opts)# => TypeError (no implicit conversion of Hash into String)
To address that, I've changed the StringIO initialization in Ruby < 2.7, so that it:
ignores options that are meant only for CSV.new
raises an ArgumentError if anything else is passed as options to CSV.open
initializes StringIO without keyword arguments.

OK. Let's use the approach.

kou · 2024-08-05T05:31:34Z

Thanks.

## Overview Previously, the first parameter name was inconsistent across methods and their RDoc comments, with names such as `path`, `source`, and `filename_or_io` used interchangeably despite referring to the same concept: a file path or an IO object. This change aligns all class methods to use `path_or_io`. This more accurately conveys that the argument accepts either a file path or an IO object. ## Reference - #302

kou reviewed May 4, 2024
View reviewed changes

Comment threadlib/csv.rb Outdated

kou changed the title ~~Allow CSV.foreach with StringIO argument~~Allow CSV.open with StringIO argumentMay 4, 2024

kou reviewed May 17, 2024
View reviewed changes

Comment threadtest/csv/interface/test_read.rb Outdated

MarcPer requested a review from kou May 24, 2024 08:12

kou reviewed May 24, 2024
View reviewed changes

kou reviewed May 25, 2024
View reviewed changes

Comment threadtest/csv/interface/test_read.rb Outdated
Comment threadtest/csv/interface/test_read.rb Outdated
Comment threadtest/csv/interface/test_read.rb Outdated

kou reviewed Jun 3, 2024
View reviewed changes

Comment threadtest/csv/interface/test_read.rb Outdated

kou reviewed Jun 4, 2024
View reviewed changes

Comment threadtest/csv/interface/test_read.rb Outdated

MarcPerand others added 11 commits August 1, 2024 17:28

Allow CSV.foreach with StringIO argument
caf27c8

When a StringIO is passed as argument to CSV.open, create a new one i…
fb8740c
…nstead of reusing it

Declares test that uses ractor
374788f
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Add test for StringIO with non-zero pos
b5bcbb0

Read full StringIO content in CSV.open, handle BOM by default
7a0d386

Simplify tests and use more descriptive variable names
88d1c77

Simplify foreach test
dbc6e35

Remove garbage
1157cf3

Skip Windows BOM bypass if opening a StringIO
ad3fcd4

Don't run StringIO tests in Ruby versions < 2.7
e089151

kou reviewed Aug 2, 2024
View reviewed changes

Use StringIO options only in Ruby >= 2.7
e499e6c

kou reviewed Aug 3, 2024
View reviewed changes

MarcPer added 2 commits August 4, 2024 13:02

Raise error when unsupported options are given to open a StringIO
f2d5b92

Initialize StringIO without options in Ruby < 2.7
74a9e0d

kou reviewed Aug 5, 2024
View reviewed changes

Comment threadlib/csv.rb Outdated
Comment threadlib/csv.rb Outdated

Adhere to existing code conventions
3f94fb4

kou reviewed Aug 5, 2024
View reviewed changes

Comment threadlib/csv.rb Outdated

Adjust style
5bab5e2

kou merged commit ce91198 into ruby:masterAug 5, 2024

alstrocrack mentioned this pull request May 4, 2026
Unify first parameter name to path_or_io across methods #358
Merged

alstrocrack mentioned this pull request May 24, 2026
CSVクラスの特定のメソッドがStringIOを受け取れることを明記したい rurema/doctree#3026
Closed

-pos=filename_or_io.pos
-f=StringIO.new(filename_or_io.read)
-filename_or_io.seek(pos)
+f=StringIO.new(filename_or_io.string)

-ifRUBY_VERSION >= "2.7"
-# Support to StringIO was dropped for Ruby 2.6 and earlier:
-# https://github.com/ruby/stringio/pull/47
-deftest_foreach_stringio
-string_io=StringIO.new(@data)
-rows=CSV.foreach(string_io,col_sep: "\t",row_sep: "\r\n").to_a
-assert_equal(@rows,rows)
-end
-deftest_foreach_stringio_with_bom
-string_io=StringIO.new("\ufeff#{@data}")# U+FEFF ZERO WIDTH NO-BREAK SPACE
-rows=CSV.foreach(string_io,col_sep: "\t",row_sep: "\r\n").to_a
-assert_equal(@rows,rows)
-end
-end
+deftest_foreach_stringio
+string_io=StringIO.new(@data)
+rows=CSV.foreach(string_io,col_sep: "\t",row_sep: "\r\n").to_a
+assert_equal(@rows,rows)
+end
+deftest_foreach_stringio_with_bom
+ifRUBY_VERSION < "2.7"
+# Support to StringIO was dropped for Ruby 2.6 and earlier without BOM support:
+# https://github.com/ruby/stringio/pull/47
+omit("StringIO's BOM support isn't available with Ruby < 2.7")
+end
+string_io=StringIO.new("\ufeff#{@data}")# U+FEFF ZERO WIDTH NO-BREAK SPACE
+rows=CSV.foreach(string_io,col_sep: "\t",row_sep: "\r\n").to_a
+assert_equal(@rows,rows)
+end

	file_opts.merge!(options)
	unlessfile_opts.key?(:newline)
	file_opts[:universal_newline] \|\|= false
	end

Uh oh!

Conversation

MarcPer commented May 4, 2024• edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcPerMay 25, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kou commented Jun 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcPer commented Jun 5, 2024

Uh oh!

kou commented Jun 6, 2024

Uh oh!

MarcPer commented Jun 6, 2024

Uh oh!

MarcPer commented Jun 13, 2024

Uh oh!

kou commented Jun 14, 2024

Uh oh!

MarcPer commented Jun 14, 2024

Uh oh!

kou commented Jun 16, 2024

Uh oh!

MarcPer commented Aug 1, 2024

Uh oh!

kouAug 2, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kou commented Aug 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MarcPer commented May 4, 2024•
edited by kou
Loading

MarcPerMay 25, 2024•
edited
Loading

kou commented Jun 4, 2024•
edited
Loading

kouAug 2, 2024•
edited
Loading