Skip to content

Conversation

@folkertdev
Copy link
Contributor

Uses unreleased zlib-rs code, so we'll have to wait with that.

But before I make a release: does this look right? Are there any other missing things?

@folkertdev
Copy link
Contributor Author

So, should zlib-rs enable any_zlib? I don't think it should, although the feature names get really confusing that way.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive support for zlib-rs features, including dictionary support and dynamic compression level adjustment. The changes integrate unreleased zlib-rs functionality and improve feature flag documentation.

Key Changes

  • Added set_dictionary() methods for both compression and decompression with zlib-rs backend
  • Added set_level() method support for zlib-rs backend with proper feature gating
  • Enhanced feature flag documentation in Cargo.toml with comprehensive descriptions of each backend option
  • Improved total_in/total_out byte tracking to exclude dictionary bytes in zlib-rs backend

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/mem.rs Added zlib-rs-specific set_dictionary and refactored set_level with feature-gated implementations for both zlib-rs and C backends
src/lib.rs Updated backend documentation to mention feature flags and added feature selection priority order documentation
src/ffi/zlib_rs.rs Implemented custom total_in/total_out tracking to exclude dictionary bytes, added dictionary and level setting methods with proper error handling
Cargo.toml Switched to unreleased zlib-rs git dependency, added document-features dependency, and comprehensively documented all feature flags with usage guidance

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Byron
Copy link
Member

Byron commented Dec 10, 2025

Thanks so much @folkertdev for getting this going!

I can't really tell if it's good like this, but can say 'it looks good'. Thus, I think we'd want some more validation.
First of all, could you rebase onto #520 (which I am trying to get merged as well), and squash your commits into one so it's easier to see what you changed?

My idea here would be to adjust the zlib-rs-capabilities test to extend to the newly added functions. That way we'd know that they are actually available with whatever feature configuration we chose. From there, we can probably find a way to adjust the documentation of the internal feature toggles, or change them entirely as they are non-public.

I hope that makes sense, if not, let's make adjustments until it does :).

@Byron Byron force-pushed the copilot/add-tests-for-zlib-rs-functionality branch from d8a67e7 to 816bd26 Compare December 10, 2025 04:13
@folkertdev folkertdev force-pushed the zlib-rs-round-2 branch 4 times, most recently from a637889 to fc06575 Compare December 10, 2025 11:46
@folkertdev
Copy link
Contributor Author

I've made the zlib-rs release now, so from that perspective this is ready. The cfg rules are a bit unfortunate to satisfy cargo clippy --all-features, but I don't see a good alternative there.

At this point, what does that separate zlib-rs test file add?

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