docs: fix compression level range from 0-9 to 0-10

[ByteBaker]

Also store the min, max, and default levels as constants. While calling Compression::new, assert that level is in range.

What's the PR for?

The docs say that compression level falls in the range 0-9 (inclusive), however, calling Compression::new(10) works, and there's an assertion, debug_assert!(level.level() <= 10); inside ffi::rust::DeflateBackend::make which ensures the limit is 10.

Hence the documentation is wrong. I've fixed the documentation as well as make a few minor changes.

[Byron]

I don't know where the idea is coming from, so 10 being a valid value would need links to a reference.

For all I could find, both zlib and zlib-ng use a documented range of 0 - 9.

[jongiddy]

miniz does appear to support level 10, calling it uber compression.

Given current flate2 support for multiple backends, we may have avoided supporting such a backend-specific setting. But removing the level 10 is backwards-incompatible. We need to test how level 10 is treated by other backends. Hopefully it changes the setting to 9.

In any case the docs should say that flate2 always supports level 0-9 with the usual zlib meanings. Level 10 is supported for miniz uber compression, and its behavior on other backends is either undefined or whatever is found with testing.

[ByteBaker]

I think my primary motivation came from seeing the docs itself. The documented range was different from what I found working. I didn't switch backend, and no feature flag was used other than anything out of the box. Additionally, there's no documentation for difference in compression levels coming from various backends.

I think the word typically in the docs is the key here.

[Byron]

Thanks for chiming in!

It seems we have collected all the information here that would be needed to explain 'typically' in detail: zlib-ng and libz support levels 0-9 and miniz support 0-10 .

I don't think any non-comment change is needed for this to be an improvement worth merging, and would appreciate if the current code-changes were removed.

When documenting best() one could probably mention that the highest value of 9 is returned here, despite 10 also being supported by miniz.

[ByteBaker]

Noted. I'll make the necessary change and document how the compression level range changes with the choice of compression backend.

Is it okay if I keep the code structure the way I have written; while changing the MAX value based on feature flag? Also, using Self instead of Compression inside impl Compression? I think the latter is more idiomatic.

[ByteBaker]

There's also another way of going about it. I could just leave things as they are, and add more context to the comment highlighting that MAX is always returned as 9 but that's just because we want to be consistent and that even though max returns 9, with miniz backend, compression level up to 10 is supported.

Which version of the solution do you think is better?

[Byron]

Let's go with a docs-only change please, thank you.

[ByteBaker]

I've updated the PR. Please review.

[Byron]

Thanks a lot!

When reading this and seeing the 0-10 range, I kept thinking that maybe this UBER-compression might not be practical as it might come at the expense of extreme time or memory usage. Thus, advertising it like 10 is a number you should choose for best compression is wrong.

So I'd be more comfortable to separate the 10 just like miniz does, so 0-9 is 0-best in all of them, and 10 is a single possible value for miniz.

Thus one could write about it like "on a scale of 0-9, but for miniz compression level 10 is also possible".

Does that make sense?

[ByteBaker]

It does. Lemme make the necessary updates.

[Byron]

Thanks a lot for bearing with me here. Now it seems to capture all we know, adding more context to the word typically.

[Shnatsel]

miniz documentation states that its level 10 is not Zlib-compatible. I understand it will produce streams that cannot be decoded by anything but miniz_oxide. As such, I don't think this compression level should be exposed by flate2 at all.

[Byron]

It's too bad I didn't scroll right when looking at this, as otherwise I would have assured this information is passed on. Right now flate2 documents it like 10 is a better 9.
Unfortunately, when creating a PR I can't merge it myself (nor when I am the last author) - could anybody submit a documentation fix to warn about the side-effects of using 10?

[ByteBaker]

Would it work if in addition to the work I did earlier I mention this warning in the docs, or should I take a different approach?

[Shnatsel]

We got it sorted in #430