Skip to content
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

Merged
merged 4 commits into from
Jun 2, 2017
Merged

Implement Zlib::GzipReader #ungetbyte and #ungetc #4636

merged 4 commits into from
Jun 2, 2017

Conversation

haines
Copy link
Contributor

@haines haines commented May 31, 2017

I've had a stab at implementing the unget* methods for Zlib::GzipReader. The implementation is a bit limited in that the maximum number of bytes that can be "ungot" is hardcoded, since I'm using a PushbackInputStream. Also, it doesn't attempt to do anything clever with encoding (although I don't see that as a problem because getc doesn't either).

Let me know what you think!

Closes #4631

@haines
Copy link
Contributor Author

haines commented May 31, 2017

I think the MRI zlib test failure (here) is actually an issue with the implementation of gets("") -- MRI skips the leading newline but JRuby does not.

@headius
Copy link
Member

headius commented May 31, 2017

@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.*;
Copy link
Member

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.

@@ -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
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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. 😄

@headius headius added this to the JRuby 9.1.11.0 milestone May 31, 2017
@haines
Copy link
Contributor Author

haines commented May 31, 2017

I've added the specs, which caught a bug in my implementation (I forgot to decrement pos), and a couple in MRI:

  • ungetting at the start of the stream causes pos to underflow (I've raised this: https://bugs.ruby-lang.org/issues/13616)
  • ungetting nil raises a TypeError: no implicit conversion of nil. I'm not sure if I should raise this as a bug in MRI or leave the behaviour unspecified. File accepts nil and silently ignores it, so I think GzipReader should behave in the same way for consistency. On the other hand, trying to unget nil is a strange thing to do, so raising a TypeError is fairly reasonable. What do you think?

I'll take a look at the #gets implementation tomorrow (UK time).

@haines
Copy link
Contributor Author

haines commented Jun 1, 2017

I think this is good to go (assuming you're happy with it!). Turns out #gets("") is supposed to skip any number of leading or trailing newlines, so I've added a spec for that behaviour too.

@headius
Copy link
Member

headius commented Jun 2, 2017

Looks good now...I'll merge!

@headius headius merged commit 15899bb into jruby:master Jun 2, 2017
@haines haines deleted the gzip_reader_unget branch June 2, 2017 19:59
haines added a commit to haines/tar that referenced this pull request Jun 6, 2017
@eregon
Copy link
Member

eregon commented Jun 23, 2017

@headius @haines

ungetting nil raises a TypeError: no implicit conversion of nil. I'm not sure if I should raise this as a bug in MRI or leave the behaviour unspecified. File accepts nil and silently ignores it, so I think GzipReader should behave in the same way for consistency. On the other hand, trying to unget nil is a strange thing to do, so raising a TypeError is fairly reasonable. What do you think?

Either follow what MRI does, or raise a bug, but please not spec different behavior without a bug report.
I cannot keep these specs in ruby/spec since they don't pass on MRI and there is no matching MRI bug.
I'll write a bug report and put these specs in quarantine in the meantime.

@eregon
Copy link
Member

eregon commented Jun 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants