Implement Zlib::GzipReader #ungetbyte and #ungetc #4636
Conversation
|
I think the MRI zlib test failure (here) is actually an issue with the implementation of |
|
@haines You may be right. Do you want to look into it? It may just be some missing logic in our ported GzipReader#gets implementation. I suppose the test passed before because it fell back on io#ungetc which didn't actually unget anything, so the subsequent gets worked properly. Thanks for your help! I have some review comments I'll add. |
| import org.jruby.RubyIO; | ||
| import org.jruby.RubyNumeric; | ||
| import org.jruby.RubyString; | ||
| import org.jruby.*; |
There was a problem hiding this comment.
We generally don't collapse imports unless there's >20 or something.
test/jruby/test_zlib.rb
Outdated
| @@ -517,6 +517,24 @@ def test_error_input | |||
| assert_equal("not in gzip format", e.message) | |||
| assert_equal("foobarzothoge", e.input) | |||
| end | |||
|
|
|||
| def test_gzip_reader_ungetc | |||
There was a problem hiding this comment.
If these don't exist in either MRI's suite or ruby/spec, it would be nice to add them there. We usually just put JRuby-specific stuff in test/jruby.
There was a problem hiding this comment.
Makes sense. There are placeholders for these methods in ruby/spec, so I'll add them there.
Should I submit them to ruby/spec first and then pull them in once merged upstream?
There was a problem hiding this comment.
You can submit them to our spec/ruby subdir. We merge both ways.
There was a problem hiding this comment.
Make it a separate commit for sure though...makes it easier to merge. 😄
|
I've added the specs, which caught a bug in my implementation (I forgot to decrement
I'll take a look at the |
|
I think this is good to go (assuming you're happy with it!). Turns out |
|
Looks good now...I'll merge! |
The test suite requires jruby/jruby#4636
Either follow what MRI does, or raise a bug, but please not spec different behavior without a bug report. |
I've had a stab at implementing the
unget*methods forZlib::GzipReader. The implementation is a bit limited in that the maximum number of bytes that can be "ungot" is hardcoded, since I'm using aPushbackInputStream. Also, it doesn't attempt to do anything clever with encoding (although I don't see that as a problem becausegetcdoesn't either).Let me know what you think!
Closes #4631