From 5e888754dea22a69ff60b92d739c06e4c498e20d Mon Sep 17 00:00:00 2001 From: Jordan Doyle Date: Sat, 20 Jan 2018 15:30:58 +0000 Subject: [PATCH] Clean up codebase a little bit, remove all unwraps and throw proper errors --- Cargo.toml | 1 - src/device.rs | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------- src/discovery.rs | 14 ++++++-------- src/error.rs | 5 +++++ src/lib.rs | 2 +- tests/integration_test.rs | 29 +++++++++++++++++++++++------ 6 files changed, 71 insertions(+), 53 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9cfee07..a4f6c2e 100644 --- a/Cargo.toml +++ a/Cargo.toml @@ -9,7 +9,6 @@ readme = "README.md" [dependencies] -clippy = {version = "0.0", optional = true} reqwest = "0.8" log = "0.3" ssdp = "0.6" diff --git a/src/device.rs b/src/device.rs index cff5fff..9fbad53 100644 --- a/src/device.rs +++ a/src/device.rs @@ -6,6 +6,7 @@ use std::io::Read; use std::time::Duration; use error::*; +pub(crate) use self::xmltree::ParseError; use self::xmltree::Element; use self::reqwest::header::{ContentType, Headers}; use self::regex::Regex; @@ -43,7 +44,6 @@ Transitioning, } - lazy_static! { static ref COORDINATOR_REGEX: Regex = Regex::new(r"^https?://(.+?):1400/xml") .expect("Failed to create regex"); @@ -51,7 +51,7 @@ /// Get the text of the given element as a String fn element_to_string(el: &Element) -> String { - el.text.to_owned().unwrap() + el.text.to_owned().unwrap_or_default() } impl Speaker { @@ -64,7 +64,7 @@ return Err(ErrorKind::BadResponse.into()); } - let elements = Element::parse(resp).unwrap(); + let elements = Element::parse(resp)?; let device_description = elements .get_child("device") .chain_err(|| ErrorKind::ParseError)?; @@ -123,19 +123,17 @@ .chain_err(|| ErrorKind::ParseError)?; // get the group identifier from the given player - let group = zone_players + let group = &zone_players .children .iter() - .find(|ref child| child.attributes.get("uuid").unwrap() == &self.uuid) + .find(|child| child.attributes["uuid"] == self.uuid) .chain_err(|| ErrorKind::DeviceNotFound(self.uuid.to_string()))? - .attributes - .get("group") - .unwrap(); + .attributes["group"]; Ok(COORDINATOR_REGEX .captures(zone_players.children.iter() // get the coordinator for the given group - .find(|ref child| + .find(|child| child.attributes.get("coordinator").unwrap_or(&"false".to_string()) == "true" && child.attributes.get("group").unwrap_or(&"".to_string()) == group) .chain_err(|| ErrorKind::DeviceNotFound(self.uuid.to_string()))? @@ -169,7 +167,11 @@ headers.set_raw("SOAPAction", format!("\"{}#{}\"", service, action)); let client = reqwest::Client::new(); - let coordinator = if coordinator { self.coordinator()? } else { self.ip }; + let coordinator = if coordinator { + self.coordinator()? + } else { + self.ip + }; debug!("Running {}#{} on {}", service, action, coordinator); @@ -193,14 +195,15 @@ .send() .chain_err(|| ErrorKind::DeviceUnreachable)?; - let element = - Element::parse(request).chain_err(|| ErrorKind::ParseError)?; + let element = Element::parse(request).chain_err(|| ErrorKind::ParseError)?; - let body = element.get_child("Body") + let body = element + .get_child("Body") .chain_err(|| ErrorKind::ParseError)?; if let Some(fault) = body.get_child("Fault") { - let error_code = element_to_string(fault.get_child("detail") + let error_code = element_to_string(fault + .get_child("detail") .chain_err(|| ErrorKind::ParseError)? .get_child("UPnPError") .chain_err(|| ErrorKind::ParseError)? @@ -213,8 +216,7 @@ error!("Got state {:?} from {}#{} call.", state, service, action); Err(ErrorKind::from(state).into()) } else { - Ok(body - .get_child(format!("{}Response", action)) + Ok(body.get_child(format!("{}Response", action)) .chain_err(|| ErrorKind::ParseError)? .clone()) } @@ -301,9 +303,7 @@ "Seek", &format!( "0REL_TIME{:02}:{:02}:{:02}", - hours, - minutes, - seconds + hours, minutes, seconds ), true, )?; @@ -432,7 +432,7 @@ .to_owned() .chain_err(|| ErrorKind::ParseError)? .parse::() - .unwrap(); + .chain_err(|| ErrorKind::ParseError)?; Ok(volume) } @@ -465,7 +465,7 @@ "MediaRenderer/RenderingControl/Control", "urn:schemas-upnp-org:service:RenderingControl:1", "GetMute", - &format!("0Master"), + "0Master", false, )?; @@ -474,8 +474,7 @@ .as_str() { "1" => true, - "0" => false, - _ => false, + "0" | _ => false, }) } @@ -520,13 +519,12 @@ .chain_err(|| ErrorKind::ParseError)?) .as_str() { - "STOPPED" => TransportState::Stopped, "PLAYING" => TransportState::Playing, "PAUSED_PLAYBACK" => TransportState::PausedPlayback, "PAUSED_RECORDING" => TransportState::PausedRecording, "RECORDING" => TransportState::Recording, "TRANSITIONING" => TransportState::Transitioning, - _ => TransportState::Stopped, + "STOPPED" | _ => TransportState::Stopped, }, ) } @@ -554,17 +552,27 @@ // convert the given hh:mm:ss to a Duration let duration: Vec = element_to_string(resp.get_child("TrackDuration") .chain_err(|| ErrorKind::ParseError)?) - .splitn(3, ":") - .map(|s| s.parse::().unwrap()) + .splitn(3, ':') + .map(|s| { + s.parse::() + .chain_err(|| ErrorKind::ParseError) + .unwrap() + }) .collect(); let duration = Duration::from_secs((duration[0] * 3600) + (duration[1] * 60) + duration[2]); let running_time: Vec = element_to_string(resp.get_child("RelTime") .chain_err(|| ErrorKind::ParseError)?) - .splitn(3, ":") - .map(|s| s.parse::().unwrap()) + .splitn(3, ':') + .map(|s| { + s.parse::() + .chain_err(|| ErrorKind::ParseError) + .unwrap() + }) .collect(); - let running_time = Duration::from_secs((running_time[0] * 3600) + (running_time[1] * 60) + running_time[2]); + let running_time = Duration::from_secs( + (running_time[0] * 3600) + (running_time[1] * 60) + running_time[2], + ); Ok(Track { title: element_to_string(metadata @@ -576,10 +584,9 @@ album: element_to_string(metadata .get_child("album") .chain_err(|| ErrorKind::ParseError)?), - queue_position: element_to_string(resp.get_child("Track") - .chain_err(|| ErrorKind::ParseError)?) + queue_position: element_to_string(resp.get_child("Track").chain_err(|| ErrorKind::ParseError)?) .parse::() - .unwrap(), + .chain_err(|| ErrorKind::ParseError)?, uri: element_to_string(resp.get_child("TrackURI") .chain_err(|| ErrorKind::ParseError)?), duration, diff --git a/src/discovery.rs b/src/discovery.rs index ba36d4d..99468d6 100644 --- a/src/discovery.rs +++ a/src/discovery.rs @@ -1,8 +1,9 @@ extern crate ssdp; use self::ssdp::FieldMap; use self::ssdp::header::{HeaderMut, HeaderRef, Man, MX, ST}; use self::ssdp::message::{Multicast, SearchRequest, SearchResponse}; +pub(crate) use self::ssdp::SSDPError; use device::Speaker; use error::*; @@ -10,11 +11,9 @@ /// Convenience method to grab a header from an SSDP search as a string. fn get_header(msg: &SearchResponse, header: &str) -> Result { - let bytes = msg.get_raw(header) - .chain_err(|| ErrorKind::ParseError)?; - - String::from_utf8(bytes.get(0).unwrap().to_vec()) - .chain_err(|| ErrorKind::ParseError) + let bytes = msg.get_raw(header).chain_err(|| ErrorKind::ParseError)?; + + String::from_utf8(bytes[0].clone()).chain_err(|| ErrorKind::ParseError) } /// Discover all speakers on the current network. @@ -29,7 +28,7 @@ let mut speakers = Vec::new(); - for (msg, src) in request.multicast().unwrap() { + for (msg, src) in request.multicast()? { let usn = get_header(&msg, "USN")?; if !usn.contains(SONOS_URN) { @@ -37,8 +36,7 @@ continue; } - speakers.push(Speaker::from_ip(src.ip()) - .chain_err(|| ErrorKind::ParseError)?); + speakers.push(Speaker::from_ip(src.ip()).chain_err(|| ErrorKind::ParseError)?); } Ok(speakers) diff --git a/src/error.rs b/src/error.rs index 59a06cc..f0b2719 100644 --- a/src/error.rs +++ a/src/error.rs @@ -25,6 +25,11 @@ display("Couldn't find a device by the given identifier ({})", identifier) } } + + foreign_links { + Discovery(::discovery::SSDPError) #[doc = "An error occurred while attempting to discover devices"]; + XMLParse(::device::ParseError) #[doc = "An error occurred while parsing device response"]; + } } impl From for ErrorKind { diff --git a/src/lib.rs b/src/lib.rs index 0025c3a..5423c73 100644 --- a/src/lib.rs +++ a/src/lib.rs @@ -19,4 +19,4 @@ pub use device::TransportState; pub use error::*; -pub use discovery::discover;+pub use discovery::discover; diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 39f1c3c..f072e4d 100644 --- a/tests/integration_test.rs +++ a/tests/integration_test.rs @@ -1,9 +1,12 @@ extern crate sonos; +use sonos::TransportState; + fn get_speaker() -> sonos::Speaker { let devices = sonos::discover().unwrap(); - devices.into_iter() + devices + .into_iter() .find(|d| d.name == "Bedroom") .ok_or("Couldn't find bedroom") .unwrap() @@ -12,7 +15,7 @@ #[test] fn can_discover_devices() { let devices = sonos::discover().unwrap(); - assert!(devices.len() > 0, "No devices discovered"); + assert!(!devices.is_empty(), "No devices discovered"); } #[test] @@ -47,22 +50,19 @@ device.play().expect("Couldn't play track"); assert!(match device.transport_state().unwrap() { - sonos::TransportState::Playing => true, - sonos::TransportState::Transitioning => true, + TransportState::Playing | TransportState::Transitioning => true, _ => false, }); device.pause().expect("Couldn't pause track"); assert!(match device.transport_state().unwrap() { - sonos::TransportState::PausedPlayback => true, - sonos::TransportState::Transitioning => true, + TransportState::PausedPlayback | TransportState::Transitioning => true, _ => false, }); device.stop().expect("Couldn't stop track"); assert!(match device.transport_state().unwrap() { - sonos::TransportState::Stopped => true, - sonos::TransportState::Transitioning => true, + TransportState::Stopped | TransportState::Transitioning => true, _ => false, }); } @@ -76,8 +76,17 @@ #[test] fn seek() { let device = get_speaker(); - device.seek(&std::time::Duration::from_secs(30)).expect("Failed to seek to 30 seconds"); - assert_eq!(device.track().expect("Failed to get track info").running_time.as_secs(), 30); + device + .seek(&std::time::Duration::from_secs(30)) + .expect("Failed to seek to 30 seconds"); + assert_eq!( + device + .track() + .expect("Failed to get track info") + .running_time + .as_secs(), + 30 + ); } #[test] -- rgit 0.1.3