Allow CSV.open with StringIO argument #302

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

lib/csv.rb

test/csv/interface/test_read.rb

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

lib/csv.rb

+        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:

def test_foreach_stringio_with_bom s = StringIO.new s << "\ufeff" # BOM s << @data s.rewind s.read(3) rows = CSV.foreach(s, col_sep: "\t", row_sep: "\r\n").to_a assert_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 ?

test/csv/interface/test_read.rb

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 StringIO was 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

test/csv/interface/test_read.rb

+  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

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

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

if RUBY_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

lib/csv.rb

+        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)

unless file_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)

unless file_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.

lib/csv.rb

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

Thanks.

kou reviewed May 4, 2024

View reviewed changes

lib/csv.rb Outdated Show resolved Hide resolved

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

kou reviewed May 17, 2024

View reviewed changes

test/csv/interface/test_read.rb Outdated Show resolved Hide resolved

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

test/csv/interface/test_read.rb Outdated Show resolved Hide resolved

test/csv/interface/test_read.rb Outdated Show resolved Hide resolved

test/csv/interface/test_read.rb Outdated Show resolved Hide resolved

kou reviewed Jun 3, 2024

View reviewed changes

test/csv/interface/test_read.rb Outdated Show resolved Hide resolved

kou reviewed Jun 4, 2024

View reviewed changes

test/csv/interface/test_read.rb Outdated Show resolved Hide resolved

MarcPer and 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

lib/csv.rb Outdated Show resolved Hide resolved

lib/csv.rb Outdated Show resolved Hide resolved

Adhere to existing code conventions

3f94fb4

kou reviewed Aug 5, 2024

View reviewed changes

lib/csv.rb Outdated Show resolved Hide resolved

Adjust style

5bab5e2

kou merged commit ce91198 into ruby:master Aug 5, 2024
46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow CSV.open with StringIO argument #302

Allow CSV.open with StringIO argument #302

MarcPer commented May 4, 2024 •

edited by kou

Loading

kou May 24, 2024

MarcPer May 24, 2024

MarcPer May 24, 2024

kou May 24, 2024

MarcPer May 25, 2024 •

edited

Loading

kou May 25, 2024

MarcPer Jun 3, 2024

kou commented Jun 4, 2024 •

edited

Loading

MarcPer commented Jun 5, 2024

kou commented Jun 6, 2024

MarcPer commented Jun 6, 2024

MarcPer commented Jun 13, 2024

kou commented Jun 14, 2024

MarcPer commented Jun 14, 2024

kou commented Jun 16, 2024

MarcPer commented Aug 1, 2024

kou Aug 2, 2024 •

edited

Loading

MarcPer Aug 3, 2024

kou Aug 3, 2024

MarcPer Aug 4, 2024

kou Aug 4, 2024

MarcPer Aug 4, 2024

kou Aug 5, 2024

kou commented Aug 5, 2024

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

Allow CSV.open with StringIO argument #302

Allow CSV.open with StringIO argument #302

Conversation

MarcPer commented May 4, 2024 • edited by kou Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcPer May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Jun 4, 2024 • edited Loading

MarcPer commented Jun 5, 2024

kou commented Jun 6, 2024

MarcPer commented Jun 6, 2024

MarcPer commented Jun 13, 2024

kou commented Jun 14, 2024

MarcPer commented Jun 14, 2024

kou commented Jun 16, 2024

MarcPer commented Aug 1, 2024

kou Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Aug 5, 2024

MarcPer commented May 4, 2024 •

edited by kou

Loading

MarcPer May 25, 2024 •

edited

Loading

kou commented Jun 4, 2024 •

edited

Loading

kou Aug 2, 2024 •

edited

Loading