ruby / csv

CSV Reading and Writing
https://ruby.github.io/csv/
BSD 2-Clause "Simplified" License
177 stars 113 forks source link

Allow CSV.open with StringIO argument #302

Closed MarcPer closed 2 weeks ago

MarcPer commented 3 months ago

Fix #300

kou commented 2 months ago

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

MarcPer commented 2 months ago

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

Thank you, I've rebase it now.

kou commented 2 months ago

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

MarcPer commented 2 months ago

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 commented 2 months ago

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

kou commented 2 months ago

Hmm. It should be fixed by https://github.com/ruby/csv/commit/47bf76fe7fb3f990802519a2f3af96fbcdc432b5 . Could you rebase again?

MarcPer commented 2 months ago

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

kou commented 2 months ago

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 commented 3 weeks ago

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.

kou commented 2 weeks ago

Thanks.