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

Make flate2 a feature flag #563

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fintelia
Copy link
Contributor

Right now this crate ships with both fdeflate and flate2 as dependencies. There's a lot of duplicated code between them including two full decompressors and a chunk of duplicated logic on the encode path. If fdeflate gains a general purpose compressor, it would be nice for us to remove the duplication by default.

This PR moves the user-facing guarantee that flate2 is used as the compression backend behind a non-default feature flag. Internally, flate2 is still used for all compression levels other than NoCompression and FdeflateUltraFast.

@fintelia fintelia requested a review from Shnatsel December 30, 2024 21:01
@Shnatsel
Copy link
Contributor

I agree with the motivation. However, I find this particular implementation rather confusing from the API user perspective. For the most common use case of opting in to zlib-rs for faster compression than miniz_oxide, the steps to enable that really aren't clear.

In the spirit of making easy things easy and hard things possible, I think we should just outright rename Flate2 variant to Level and add zlib-rs toplevel feature that enables zlib-rs for encoding. All the other uses for messing with flate2 are sufficiently niche that they're not worth complicating the API surface for everyone.

@Shnatsel
Copy link
Contributor

I'll make a PR for the option I suggested and we can see if that's any better

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.

3 participants