csv icon indicating copy to clipboard operation
csv copied to clipboard

Allow CSV.open with StringIO argument

Open MarcPer opened this issue 1 year ago • 8 comments

Fix #300

MarcPer avatar May 04 '24 11:05 MarcPer

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

kou avatar Jun 04 '24 21:06 kou

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

Thank you, I've rebase it now.

MarcPer avatar Jun 05 '24 06:06 MarcPer

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

kou avatar Jun 06 '24 01:06 kou

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 avatar Jun 06 '24 14:06 MarcPer

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

MarcPer avatar Jun 13 '24 20:06 MarcPer

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

kou avatar Jun 14 '24 00:06 kou

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

MarcPer avatar Jun 14 '24 09:06 MarcPer

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)

kou avatar Jun 16 '24 21:06 kou

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.

MarcPer avatar Aug 01 '24 15:08 MarcPer

Thanks.

kou avatar Aug 05 '24 05:08 kou