Add option to set the binary mode of the log device

[rafaelfranca]

Without binmode strings with incompatible encoding can't be written in
the file. This is very common in applications that log user provided
parameters.

We need to allow changing the binnary mode because right now it is impossible to use
the built-in log rotation feature when you provide a File object to the
LogDevice, and if you provide a filename you can't have binmode.

[rafaelfranca]

cc @sonots @hsbt @tenderlove

[sonots]

Does changing defaults to binmode break any backward compatibilities? For example, what if a log file is already written with previous version of loggers?

If there are potential risks breaking backward compatibilities, I prefer adding another option.

[amatsuda]

Let's summon @nobu for this kind of quiz.

[rafaelfranca]

It seems there is no backward incompatibility between the two implementation. Both creating a file using binmode and appending to it without binmode and, creating a file without binmode and appending to it with binmode work fine.

>> f = File.open("tmp.txt", (File::WRONLY | File::APPEND | File::CREAT | File::EXCL))
=> #<File:tmp.txt>
>> f.binmode
=> #<File:tmp.txt>
>> f.write("\x80".force_encoding("ASCII-8BIT"))
=> 1
>> f.close
=> nil
>> f = File.open("tmp.txt", (File::WRONLY | File::APPEND))
=> #<File:tmp.txt>
>> f.write("a")
=> 1
>> f.close
=> nil
>> f = File.open("not_binmode.txt", (File::WRONLY | File::APPEND | File::CREAT | File::EXCL))
=> #<File:not_binmode.txt>
>> f.write("a")
=> 1
>> f.close
=> nil
>> f = File.open("not_binmode.txt", (File::WRONLY | File::APPEND))
=> #<File:not_binmode.txt>
>>
>> f.write("\x80".force_encoding("ASCII-8BIT"))
=> 1
>> f.close
=> nil

[ioquatix]

This looks good, the only question I'd ask is should we change the mode of the file if it's supplied by the user?

[rafaelfranca]

This looks good, the only question I'd ask is should we change the mode of the file if it's supplied by the user?

I think that should be up to the user. They should be able to pass a file using any mode they want.

[ioquatix]

Okay one last point - do you want to update the documentation too so users know what's going on?

[rafaelfranca]

Yeah, that one I believe we should. I'll take a look.

[rafaelfranca]

Done

[ioquatix]

LGTM, but I'll let @sonots merge it.

[sonots]

As far as I know, on windows, binmode disables newline conversion. It breaks backward compatible behaviors, doesn't it?

[sonots]

Also, someone may rely on encoding conversion feature of the text mode like writing source codes and log texts in EUC-JP and outputting logs with EUC-JP on Linux and Shift_JIS on windows using the text mode feature. (although I believe recent Japanese uses only UTF-8 on both environment).

[sonots]

I agree the preference making defaults to binmode, I definitely change to binmode if this is my own personal gem. But, I am very conservative about this ruby standard library.

[nobu]

I don't see any failure in your test_write_binary_data, with the current ruby.

[sonots]

Decided. Could you provide a new option like binmode: true and default to false not to break backward compatibility?

[nobu]

logfile is left open.
Use File.binread.

[rafaelfranca]

👍

[rafaelfranca]

Could you provide a new option like binmode: true and default to false not to break backward compatibility?

Of course. Working on it.

[rafaelfranca]

Changed this to take an option and kept the default as it was.

[sonots]

Sorry to be late. LGTM

[rafaelfranca]

Thank you!

[ioquatix]

This was a great example of everyone working together and understanding each others requirements. Thanks for being great software engineers and human beings!