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

Expose zlib options on cloudflare-zlib #230

Merged
merged 2 commits into from
Mar 2, 2020

Conversation

RReverser
Copy link
Contributor

@alexcrichton
Copy link
Member

Thanks! On second thought, though, could the cloudflare_zlib implicitly enable the zlib feature? That should avoid any sort of duplication here in theory.

@RReverser
Copy link
Contributor Author

On second thought, though, could the cloudflare_zlib implicitly enable the zlib feature? That should avoid any sort of duplication here in theory.

Hmm, maybe. Then you'll need something like cfg(feature = "zlib", not(feature = "cloudflare_zlib")) and such on actual imports, right?

@RReverser
Copy link
Contributor Author

No, wait, then it will still include libz-sys by accident... Unless there is some elegant approach that I'm missing.

@RReverser
Copy link
Contributor Author

The only wait to avoid that seems to be something like:

any_zlib = []
zlib = ["any_zlib", "libz-sys"]
cloudflare_zlib = ["any_zlib", "cloudflare-zlib-sys"]

And then making conditions based on any_zlib instead. Wdyt?

@RReverser
Copy link
Contributor Author

@alexcrichton Somewhat related. I've noticed that higher-level APIs currently don't expose these advanced zlib configurations, which seems a shame because they're much easier to use than Compress / Decompress directly, and sometimes you really need that fine-tuning.

Would you be open to adding either new methods to ZlibEncoder / ZlibDecoder to propagate corresponding params, or adding implementations of From<Compress> / From<Decompress> so that one could create a high-level wrapper for a preconfigured instance?

@alexcrichton
Copy link
Member

Well however it may be done I would prefer to avoid having to write any(...) everywhere. Whether that's done by activating the zlib feature or having something like any_zlib I don't particularly mind.

In terms of adding higher-level APIs, yes, that seems fine to me too. I would prefer it if were a separate PR though and for this to not balloon too much.

@RReverser
Copy link
Contributor Author

I would prefer it if were a separate PR though and for this to not balloon too much.

Oh sure, absolutely.

@RReverser
Copy link
Contributor Author

Added an any_zlib feature group as described above.

@alexcrichton alexcrichton merged commit 5ef8702 into rust-lang:master Mar 2, 2020
@alexcrichton
Copy link
Member

👍

@RReverser RReverser deleted the cloudflare-zlib-config branch March 4, 2020 15:08
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.

2 participants