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
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

/// Do not compress.
#[deprecated(since = "0.5.1", note = "libbz2 does not support compresson level 0")]
pub fn none() -> Compression {
Compression(0)
}

/// Create a new compression spec with a specific numeric level in the range `1..=9`.
pub const fn try_new(level: u32) -> Option<Compression> {
if level >= 1 && level <= 9 {
Some(Compression(level))
} else {
None
}
}

/// Optimize for the best speed of encoding.
pub fn fast() -> Compression {
pub const fn fast() -> Compression {
Compression(1)
}

/// Optimize for the size of data being encoded.
pub fn best() -> Compression {
/// Optimize for smallest output size.
pub const fn best() -> Compression {
Compression(9)
}

/// Return the compression level as an integer.
pub fn level(&self) -> u32 {
pub const fn level(&self) -> u32 {
self.0
}
}
Expand Down
Loading