-
Notifications
You must be signed in to change notification settings - Fork 82
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
Enable customizing Zstd decoding parameters. #285
Conversation
@@ -16,6 +16,16 @@ impl ZstdDecoder { | |||
} | |||
} | |||
|
|||
pub(crate) fn new_with_params(params: &[crate::zstd::DParameter]) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should propagate the error from zstd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not necessarily opposed to this, though in the interest of consistency, it appears that this library chooses to panic on construction errors for other encoders/decoders:
pub(crate) fn new_with_params(level: i32, params: &[crate::zstd::CParameter]) -> Self { Xz2FileFormat::Xz => Stream::new_easy_encoder(level, Check::Crc64).unwrap(),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's unfortunate, maybe we should add a try_
variant for these APIs?
cc @robjtede what's your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think typically panics are acceptable when it comes to "programmer error" and I really, really don't expect decoding parameters to be user-input, or anything other than hardcoded. Therefore it's fine to me that this can panic. It's a fairly unrecoverable error for both these unwraps.
dc585ee
to
fafcde9
Compare
This change is technically semver-compatible with only a minor version bump on next release, though it's unclear to me how this would be compatible with a crate that locks |
Given that we don't expose any implementation detail, I think it can be said as semver-compatible API-wise |
yes nice change, thanks for the work :) |
closes #284
This PR updates the minimum
zstd
version to0.13.1
, in order to makeDParameter
Clone+Copy
.