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

Support decompressor plugins #427

Merged
merged 21 commits into from Feb 1, 2020

Conversation

jspanjers
Copy link
Contributor

@jspanjers jspanjers commented Jan 5, 2020

As suggested in #425, this pull request makes it possible to easily extend rubyzip with additional decompressors.

@coveralls
Copy link

coveralls commented Jan 5, 2020

Coverage Status

Coverage increased (+0.2%) to 95.674% when pulling 0b9433c on jspanjers:refactor-decompressor into 0b79104 on rubyzip:master.

Copy link
Member

@jdleesmiller jdleesmiller left a comment

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.

lib/zip/inflater.rb Outdated Show resolved Hide resolved
lib/zip/inflater.rb Outdated Show resolved Hide resolved
end

def produce_input
@decompressor.produce_input
@decompressor.read(CHUNK_SIZE)
Copy link
Member

@jdleesmiller jdleesmiller Jan 25, 2020

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_SIZEs with the same value already. I wonder whether using Decompressor::CHUNK_SIZE would be appropriate here?

Copy link
Contributor Author

@jspanjers jspanjers Jan 26, 2020

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.

@jspanjers
Copy link
Contributor Author

jspanjers commented Jan 26, 2020

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?

Agreed.

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.

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.

@hainesr
Copy link
Member

hainesr commented Jan 26, 2020

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.

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.

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.

Copy link
Member

@jdleesmiller jdleesmiller left a comment

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

@jdleesmiller jdleesmiller Feb 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rescue Zlib::Error => e
rescue Zlib::Error

@jdleesmiller jdleesmiller merged commit 666fb8c into rubyzip:master Feb 1, 2020
2 checks passed
This was referenced Feb 1, 2020
This was referenced Mar 11, 2021
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

4 participants