From a4644959b0f980d94898d6c2e3cb1763aac73a5e Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Tue, 25 Apr 2017 17:02:25 -0700 Subject: [PATCH] refactor(header): make Quality an opaque struct This makes the `u16` in `Quality` private, since it only has a valid range of 0-1000, and can't be enforced in public. The `q` function now allows both `f32`s and `u16`s to construct a `Quality`. BREAKING CHANGE: Any use of `Quality(num)` should change to `q(num)`. --- src/header/common/accept.rs | 14 ++--- src/header/common/accept_charset.rs | 6 +- src/header/common/accept_encoding.rs | 6 +- src/header/common/accept_language.rs | 8 +-- src/header/common/te.rs | 6 +- src/header/shared/quality_item.rs | 86 ++++++++++++++++++++-------- 6 files changed, 82 insertions(+), 44 deletions(-) diff --git a/src/header/common/accept.rs b/src/header/common/accept.rs index 4bbca169ab..edd18eca78 100644 --- a/src/header/common/accept.rs +++ b/src/header/common/accept.rs @@ -53,7 +53,7 @@ header! { /// ); /// ``` /// ``` - /// use hyper::header::{Headers, Accept, QualityItem, Quality, qitem}; + /// use hyper::header::{Headers, Accept, QualityItem, q, qitem}; /// use hyper::mime::{Mime, TopLevel, SubLevel}; /// /// let mut headers = Headers::new(); @@ -64,11 +64,11 @@ header! { /// qitem(Mime(TopLevel::Application, /// SubLevel::Ext("xhtml+xml".to_owned()), vec![])), /// QualityItem::new(Mime(TopLevel::Application, SubLevel::Xml, vec![]), - /// Quality(900)), + /// q(900)), /// qitem(Mime(TopLevel::Image, /// SubLevel::Ext("webp".to_owned()), vec![])), /// QualityItem::new(Mime(TopLevel::Star, SubLevel::Star, vec![]), - /// Quality(800)) + /// q(800)) /// ]) /// ); /// ``` @@ -85,18 +85,18 @@ header! { // test1, // vec![b"audio/*; q=0.2, audio/basic"], // Some(HeaderField(vec![ - // QualityItem::new(Mime(TopLevel::Audio, SubLevel::Star, vec![]), Quality(200)), + // QualityItem::new(Mime(TopLevel::Audio, SubLevel::Star, vec![]), q(200)), // qitem(Mime(TopLevel::Audio, SubLevel::Ext("basic".to_owned()), vec![])), // ]))); test_header!( test2, vec![b"text/plain; q=0.5, text/html, text/x-dvi; q=0.8, text/x-c"], Some(HeaderField(vec![ - QualityItem::new(Mime(TopLevel::Text, SubLevel::Plain, vec![]), Quality(500)), + QualityItem::new(Mime(TopLevel::Text, SubLevel::Plain, vec![]), q(500)), qitem(Mime(TopLevel::Text, SubLevel::Html, vec![])), QualityItem::new( Mime(TopLevel::Text, SubLevel::Ext("x-dvi".to_owned()), vec![]), - Quality(800)), + q(800)), qitem(Mime(TopLevel::Text, SubLevel::Ext("x-c".to_owned()), vec![])), ]))); // Custom tests @@ -112,7 +112,7 @@ header! { Some(Accept(vec![ QualityItem::new(Mime(TopLevel::Text, SubLevel::Plain, vec![(Attr::Charset, Value::Utf8)]), - Quality(500)), + q(500)), ]))); #[test] diff --git a/src/header/common/accept_charset.rs b/src/header/common/accept_charset.rs index 7e4d1247fa..5133daf6b0 100644 --- a/src/header/common/accept_charset.rs +++ b/src/header/common/accept_charset.rs @@ -29,13 +29,13 @@ header! { /// ); /// ``` /// ``` - /// use hyper::header::{Headers, AcceptCharset, Charset, Quality, QualityItem}; + /// use hyper::header::{Headers, AcceptCharset, Charset, q, QualityItem}; /// /// let mut headers = Headers::new(); /// headers.set( /// AcceptCharset(vec![ - /// QualityItem::new(Charset::Us_Ascii, Quality(900)), - /// QualityItem::new(Charset::Iso_8859_10, Quality(200)), + /// QualityItem::new(Charset::Us_Ascii, q(900)), + /// QualityItem::new(Charset::Iso_8859_10, q(200)), /// ]) /// ); /// ``` diff --git a/src/header/common/accept_encoding.rs b/src/header/common/accept_encoding.rs index cbc0de627d..e4887b4836 100644 --- a/src/header/common/accept_encoding.rs +++ b/src/header/common/accept_encoding.rs @@ -45,14 +45,14 @@ header! { /// ); /// ``` /// ``` - /// use hyper::header::{Headers, AcceptEncoding, Encoding, QualityItem, Quality, qitem}; + /// use hyper::header::{Headers, AcceptEncoding, Encoding, QualityItem, q, qitem}; /// /// let mut headers = Headers::new(); /// headers.set( /// AcceptEncoding(vec![ /// qitem(Encoding::Chunked), - /// QualityItem::new(Encoding::Gzip, Quality(600)), - /// QualityItem::new(Encoding::EncodingExt("*".to_owned()), Quality(0)), + /// QualityItem::new(Encoding::Gzip, q(600)), + /// QualityItem::new(Encoding::EncodingExt("*".to_owned()), q(0)), /// ]) /// ); /// ``` diff --git a/src/header/common/accept_language.rs b/src/header/common/accept_language.rs index 7ba3bb01d9..ec1610a5b0 100644 --- a/src/header/common/accept_language.rs +++ b/src/header/common/accept_language.rs @@ -36,15 +36,15 @@ header! { /// ``` /// # extern crate hyper; /// # #[macro_use] extern crate language_tags; - /// # use hyper::header::{Headers, AcceptLanguage, QualityItem, Quality, qitem}; + /// # use hyper::header::{Headers, AcceptLanguage, QualityItem, q, qitem}; /// # /// # fn main() { /// let mut headers = Headers::new(); /// headers.set( /// AcceptLanguage(vec![ /// qitem(langtag!(da)), - /// QualityItem::new(langtag!(en;;;GB), Quality(800)), - /// QualityItem::new(langtag!(en), Quality(700)), + /// QualityItem::new(langtag!(en;;;GB), q(800)), + /// QualityItem::new(langtag!(en), q(700)), /// ]) /// ); /// # } @@ -59,7 +59,7 @@ header! { test2, vec![b"en-US, en; q=0.5, fr"], Some(AcceptLanguage(vec![ qitem("en-US".parse().unwrap()), - QualityItem::new("en".parse().unwrap(), Quality(500)), + QualityItem::new("en".parse().unwrap(), q(500)), qitem("fr".parse().unwrap()), ]))); } diff --git a/src/header/common/te.rs b/src/header/common/te.rs index 11b7bd6711..54fc5dcb64 100644 --- a/src/header/common/te.rs +++ b/src/header/common/te.rs @@ -45,14 +45,14 @@ header! { /// ); /// ``` /// ``` - /// use hyper::header::{Headers, TE, Encoding, QualityItem, Quality, qitem}; + /// use hyper::header::{Headers, TE, Encoding, QualityItem, q, qitem}; /// /// let mut headers = Headers::new(); /// headers.set( /// TE(vec![ /// qitem(Encoding::Trailers), - /// QualityItem::new(Encoding::Gzip, Quality(600)), - /// QualityItem::new(Encoding::EncodingExt("*".to_owned()), Quality(0)), + /// QualityItem::new(Encoding::Gzip, q(600)), + /// QualityItem::new(Encoding::EncodingExt("*".to_owned()), q(0)), /// ]) /// ); /// ``` diff --git a/src/header/shared/quality_item.rs b/src/header/shared/quality_item.rs index 1f976a869a..627db5c481 100644 --- a/src/header/shared/quality_item.rs +++ b/src/header/shared/quality_item.rs @@ -4,6 +4,8 @@ use std::default::Default; use std::fmt; use std::str; +use self::internal::IntoQuality; + /// Represents a quality used in quality values. /// /// Can be created with the `q` function. @@ -19,17 +21,7 @@ use std::str; /// [RFC7231 Section 5.3.1](https://tools.ietf.org/html/rfc7231#section-5.3.1) /// gives more information on quality values in HTTP header fields. #[derive(Copy, Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] -pub struct Quality(pub u16); - -impl fmt::Display for Quality { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self.0 { - 1000 => Ok(()), - 0 => f.write_str("; q=0"), - x => write!(f, "; q=0.{}", format!("{:03}", x).trim_right_matches('0')) - } - } -} +pub struct Quality(u16); impl Default for Quality { fn default() -> Quality { @@ -67,7 +59,12 @@ impl cmp::PartialOrd for QualityItem { impl fmt::Display for QualityItem { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}{}", self.item, format!("{}", self.quality)) + try!(fmt::Display::fmt(&self.item, f)); + match self.quality.0 { + 1000 => Ok(()), + 0 => f.write_str("; q=0"), + x => write!(f, "; q=0.{}", format!("{:03}", x).trim_right_matches('0')) + } } } @@ -113,6 +110,7 @@ impl str::FromStr for QualityItem { } } +#[inline] fn from_f32(f: f32) -> Quality { // this function is only used internally. A check that `f` is within range // should be done before calling this method. Just in case, this @@ -127,10 +125,45 @@ pub fn qitem(item: T) -> QualityItem { QualityItem::new(item, Default::default()) } -/// Convenience function to create a `Quality` from a float. -pub fn q(f: f32) -> Quality { - assert!(f >= 0f32 && f <= 1f32, "q value must be between 0.0 and 1.0"); - from_f32(f) +/// Convenience function to create a `Quality` from a float or integer. +/// +/// Implemented for `u16` and `f32`. Panics if value is out of range. +pub fn q(val: T) -> Quality { + val.into_quality() +} + +mod internal { + use super::Quality; + + // TryFrom is probably better, but it's not stable. For now, we want to + // keep the functionality of the `q` function, while allowing it to be + // generic over `f32` and `u16`. + // + // `q` would panic before, so keep that behavior. `TryFrom` can be + // introduced later for a non-panicking conversion. + + pub trait IntoQuality: Sealed + Sized { + fn into_quality(self) -> Quality; + } + + impl IntoQuality for f32 { + fn into_quality(self) -> Quality { + assert!(self >= 0f32 && self <= 1f32, "float must be between 0.0 and 1.0"); + super::from_f32(self) + } + } + + impl IntoQuality for u16 { + fn into_quality(self) -> Quality { + assert!(self <= 1000, "u16 must be between 0 and 1000"); + Quality(self) + } + } + + + pub trait Sealed {} + impl Sealed for u16 {} + impl Sealed for f32 {} } #[cfg(test)] @@ -139,17 +172,17 @@ mod tests { use super::super::encoding::*; #[test] - fn test_quality_item_show1() { + fn test_quality_item_fmt_q_1() { let x = qitem(Chunked); assert_eq!(format!("{}", x), "chunked"); } #[test] - fn test_quality_item_show2() { + fn test_quality_item_fmt_q_0001() { let x = QualityItem::new(Chunked, Quality(1)); assert_eq!(format!("{}", x), "chunked; q=0.001"); } #[test] - fn test_quality_item_show3() { + fn test_quality_item_fmt_q_05() { // Custom value let x = QualityItem{ item: EncodingExt("identity".to_owned()), @@ -158,6 +191,16 @@ mod tests { assert_eq!(format!("{}", x), "identity; q=0.5"); } + #[test] + fn test_quality_item_fmt_q_0() { + // Custom value + let x = QualityItem{ + item: EncodingExt("identity".to_owned()), + quality: Quality(0), + }; + assert_eq!(x.to_string(), "identity; q=0"); + } + #[test] fn test_quality_item_from_str1() { let x: ::Result> = "chunked".parse(); @@ -201,11 +244,6 @@ mod tests { assert_eq!(q(0.5), Quality(500)); } - #[test] - fn test_quality2() { - assert_eq!(format!("{}", q(0.0)), "; q=0"); - } - #[test] #[should_panic] // FIXME - 32-bit msvc unwinding broken #[cfg_attr(all(target_arch="x86", target_env="msvc"), ignore)]