memorysafety / zlib-rs

A safer zlib
zlib License
75 stars 6 forks source link

Fix inflate() flush modes and implement state.data_type #113

Closed cjgriscom closed 3 weeks ago

cjgriscom commented 3 weeks ago

This PR address the following issues:

cjgriscom commented 3 weeks ago

I split the changes into 3 commits and added a test which compares the stream positions and state data after each call to inflate(). It demonstrates that all three changes are needed for consistency

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.59%. Comparing base (52e1628) to head (d282f90). Report is 9 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #113 +/- ## ========================================== + Coverage 89.95% 90.59% +0.63% ========================================== Files 36 36 Lines 9664 9767 +103 ========================================== + Hits 8693 8848 +155 + Misses 971 919 -52 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

folkertdev commented 3 weeks ago

After your changes, other tests were failing. Turns out this is what the LEN_ state is for. It looks really weird otherwise because it does no real work, just falls throught, but apparently that state is important for the correct behavior of Flush::Trees.

Also for the future, our CI checks formatting and runs cargo clippy (also on tests). We advise you run those tools locally at every commit, that'll save a bunch of back-and-forth with CI.

cjgriscom commented 3 weeks ago

Thanks for merging and for your work on this crate!