-
Notifications
You must be signed in to change notification settings - Fork 313
Support decompressor plugins #427
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
Conversation
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.
Thanks for the PR and sorry for the long turnaround. This looks great to me 👍. Just a few suggestions.
I think this could go out in a semver minor version bump, probably 2.2 (I am planning to release 2.1 shortly), because I don't see any breaking changes, except to classes marked with :nodoc:
. Does that sound right?
Are you planning on releasing a plugin gem with bzip2 compression support? You can of course keep that under your own namespace, but if you would like it to live under the rubyzip org, we can ask @simonoff about setting that up.
end | ||
|
||
def produce_input | ||
@decompressor.produce_input | ||
@decompressor.read(CHUNK_SIZE) |
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.
There are quite a few CHUNK_SIZE
s with the same value already. I wonder whether using Decompressor::CHUNK_SIZE
would be appropriate here?
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.
I think the optimal CHUNK_SIZE for decompressors is dependant on compression method. Thus the Decompressor::CHUNK_SIZE should not exist and be replaced by an implementation dependant CHUCK_SIZE in Inflater.
Therefore, in InputStream, I think it is better to not depend on this Decompressor::CHUNK_SIZE.
Agreed.
Yes, I am planning to create a bzip2 compression plugin gem, after this pull request is accepted. When the plugin gem is ready, I would very much like to transfer control (and maintainership) to rubyzip.org. |
This aligns Inflater#produce_input with PassThruDecompresser#produce_input.
Now, STORED files can be decrypted, just like DEFLATED files.
6f90c6e
to
0b9433c
Compare
I think developing a collection of plugins under the rubyzip org is a nice idea. It would also be an opportunity to widen the cohort of maintainers of the library and drive more participation. |
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.
Great, thanks for the updates. Spotted one minor thing but I can fix that post-merge.
rescue Zlib::BufError | ||
raise if retried >= 5 # how many times should we retry? | ||
retried += 1 | ||
retry | ||
end | ||
rescue Zlib::Error => e |
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.
rescue Zlib::Error => e | |
rescue Zlib::Error |
Wallet |
As suggested in #425, this pull request makes it possible to easily extend rubyzip with additional decompressors.