Set OutputStream.write_buffer's default buffer to binmode#439
Set OutputStream.write_buffer's default buffer to binmode#439jdleesmiller merged 3 commits intorubyzip:masterfrom
Conversation
hainesr
left a comment
There was a problem hiding this comment.
Thanks for this. Just one comment/suggestion from me.
|
|
||
| # Same as #open but writes to a filestream instead | ||
| def write_buffer(io = ::StringIO.new(''), encrypter = nil) | ||
| def write_buffer(io = ::StringIO.new('').binmode, encrypter = nil) |
There was a problem hiding this comment.
What if we pass an IO in here that hasn't been set to binmode? Do we still see the behaviour you're trying to avoid? This change only sets a newly created StringIO to binmode. Is this a problem?
How about:
| def write_buffer(io = ::StringIO.new('').binmode, encrypter = nil) | |
| def write_buffer(io = ::StringIO.new(''), encrypter = nil) | |
| io.binmode if io.respond_to?(:binmode) |
Which would match behaviour in ::Zip::File.open_buffer?
There was a problem hiding this comment.
I found another location where the binmode would be set if supported. I think this is a cleaner solution:
https://github.com/henkeinfo/rubyzip/blob/5ce4e13ddd33443f01fa33c8208423b76d99fce3/lib/zip/file.rb#L151
Yes, you're right with your comment. I rewrote the write_buffer to behave like the source above and also fixed the same thing in yet another method.
There was a problem hiding this comment.
Yes I spotted that when I was looking through File and wondered why it was inconsistent. Thanks for the catch.
This looks good to me.
f0526d7 to
b0ee268
Compare
There was a problem hiding this comment.
This looks like a good patch to me.
@jdleesmiller I can't merge this myself, obvs, so does it look OK to you too?
jdleesmiller
left a comment
There was a problem hiding this comment.
Thanks for the patch! A few comments inline.
In my case this is UTF-8, which produces encoding errors in encoding sensitive use cases.
I'd be interested to see what the actual use case was that caused the error --- if we can get a test that failed (pre-fix) in the way that motivated you to open the issue, I think that will be more likely to catch future regressions.
There have been binmode problems in the past, notably #119 / #201, which resulted in this line:
Lines 150 to 151 in 8d91d00
However, all the tests still pass even if that line is deleted, so I feel like we have already have some gaps in this area.
Co-Authored-By: John Lees-Miller <jdleesmiller@gmail.com>
|
Thank you for your feedback @jdleesmiller. My use case was an extension in my code where i filtered empty strings before storing to a database ( Lines 150 to 151 in 8d91d00 perhaps this line now is also superfluous because it internally calls OutputStream#write_buffer with my fix. But the yield call is done after this line and before the OutputStream#write_buffer, so removing this line potentially could break things.
|
jdleesmiller
left a comment
There was a problem hiding this comment.
OK, thanks for the updates. Looks good.
I think you're right that removing the existing binmode line would change what the user might get in the yield, so it's probably best to keep it.
| TEST_ZIP = TestZipFile::TEST_ZIP2.clone | ||
| TEST_ZIP.zip_name = 'test/data/generated/output.zip' | ||
|
|
||
| ASCII8BIT = 'ASCII-8BIT' |
There was a problem hiding this comment.
This constant can now be removed, but I'll tidy it up post-merge.
|
This appears to have affected an upstream gem because of the encoding changing. weshatheleopard/rubyXL#377 |
* rubyzip 2.3.0からエンコーディング結果がASCII-8BITに変更された * rubyzip/rubyzip#439
* rubyzip 2.3.0からエンコーディング結果がASCII-8BITに変更された * rubyzip/rubyzip#439
Fixes #438