ruby icon indicating copy to clipboard operation
ruby copied to clipboard

Make Encoding::Converter newline: :lf option a synonym for newline: :universal

Open jeremyevans opened this issue 4 years ago • 5 comments

Previously, newline: :lf was accepted but ignored. Where it should have been used was commented out code that didn't work, but unlike all other invalid values, using newline: :lf did not raise an error.

However, as newline: :cr converts LF to CR, it only makes sense for newline: :lf to convert CR to LF, which is the same behavior as newline: :universal. So make newline: :lf a synonym for newline: :universal.

Add tests for the File.open :newline option to test this.

Fixes [Bug #12436]

jeremyevans avatar Jun 18 '21 23:06 jeremyevans

LF-mode and Universal-mode differ at reading. Universal-mode ends a line at a CR and translates it to LF, but LF-mode doesn't.

nobu avatar Jun 19 '21 09:06 nobu

@nobu Thank you for your review. I didn't realize the difference before. That makes the patch much more involved, since it requires adding support for a new transcoder. Hopefully the new commit handles all issues. I'm not sure if the way I modified the flags is considered acceptable. If not, please let me know and I can probably change it.

jeremyevans avatar Jun 21 '21 20:06 jeremyevans

@nobu It appears that newline: :crlf converts on read and not just on write, but only on Windows. Is this a bug, or is this expected behavior on Windows?:

D:\>c:\Ruby30-x64\bin\irb
irb(main):001:0" path = "a"
=> "a"
irb(main):002:0> File.binwrite(path, "a\r\nb\r\n")
=> 6
irb(main):003:0: File.open(path, "rt", newline: :crlf, &:read)
=> "a\nb\n"
irb(main):004:0> File.open(path, "rt", newline: :cr, &:read)
=> "a\r\nb\r\n"

I'm assuming this is expected behavior and not a bug. It's unrelated to this patch. I'll push a commit that modifies the test so this test hopefully passes. I'll also rebase against master and hope that makes AppVeyor happy.

jeremyevans avatar Jun 21 '21 21:06 jeremyevans

1 failure, but it doesn't seem related to this change: https://github.com/ruby/ruby/pull/4590/checks?check_run_id=2879532690#step:15:215

jeremyevans avatar Jun 21 '21 22:06 jeremyevans

Sorry, was quite surprised when I saw Convertor first, but relieved when I found that it's actually Converter. On Google search, the later wins by about 40 times.

duerst avatar Jun 22 '21 07:06 duerst