-
-
Notifications
You must be signed in to change notification settings - Fork 925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can submit them to our spec/ruby subdir. We merge both ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 becausegetc
doesn't either).Let me know what you think!
Closes #4631