Bzip2

[jspanjers]

No description provided.

[coveralls]

Coverage increased (+0.09%) to 95.541% when pulling 39eec09 on jspanjers:bzip2 into 6389d65 on rubyzip:master.

[hainesr]

Can I ask why you copied in the bz2 ffi stuff rather than stayed using the dependency?

[jspanjers]

Can I ask why you copied in the bz2 ffi stuff rather than stayed using the dependency?

I started development depending on bzip2-ffi.

Afterwards I realized that I was using the private internal api from bzip2-ffi (Bzip2::FFI::Libbz2). I think it is not right to make rubyzip depend on non public api from bzip2-ffi.

I noticed that I was only using the Libbz2 ffi bindings, which are just 75 lines of code. Copying these bindings to rubyzip prevents the use of private api and also removes all dependencies on bzip2-ffi, leaving only a dependency on ffi.

[jdleesmiller]

Thanks for putting this together.

It would be nice if bzip2 compression worked 'out of the box'. However, having to deal with the optional ffi dependency seems a bit untidy, and adding it as a dependency just for bzip2 support seems a bit overkill, because AFAICT this is the first time this has come up in the fairly long history of this library.

I think a cleaner solution would be to have a separate 'plugin' gem for bzip2 support, e.g. rubyzip-bzip2, which could then depend on ffi (or other bzip2-related dependencies as you mentioned in #425 (comment)). I think we're about 95% of the way to being able to do that --- it's just get_decompressor that isn't currently quite general enough to for another gem to hook into.

(And some changes here, e.g. DecompressionError, might indeed be more appropriate in the core rubyzip library.)

So, I'll leave this open for comment for a while longer, but I am not currently inclined to merge it as-is. I would welcome a PR to make it easier to plug in a new decompressor, however.

[hainesr]

+1 I like/prefer the idea of a separate rubyzip-bzip2 gem for this.

[jdleesmiller]

Closing in view of #427.