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

deprecate Compression::new #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

folkertdev
Copy link
Collaborator

fixes #107

We could also just remove the bad methods with a major version bump, but this seems a little gentler and should make the problem go away in practice.

@@ -82,28 +82,39 @@ pub mod write;
pub struct Compression(u32);

impl Compression {
/// Create a new compression spec with a specific numeric level (0-9).
/// Create a new compression spec with a specific numeric level in the range `1..=9`.
#[deprecated(since = "0.5.1", note = "use try_new instead")]
pub fn new(level: u32) -> Compression {
Compression(level)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think panicking if the compression level is not in the range 1..=9 is fine. If you try to use the returned Compression value when compressing data you will get an error anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right you get an error, not a panic.

Anyhow, existing users will already not run into this (or they would have before), and now new users should never run into this behavior, and then at some point we can just drop it from the public interface.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Compression::new(2) is a much better API than Compression::try_new(2).unwrap(), so I did much rather fix Compression::new() than add a new Compression::try_new() and remove the former in the future.

Anyhow, existing users will already not run into this (or they would have before)

That is exactly why I think adding a panic in Compression::new() is fine.

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.

BzEncoder::new with Compression::none fails with -2
2 participants