Improved on the new workspace code for cargo-fmt

Author: Emilgardis

src/bin/cargo-fmt.rs

@@ -24,7 +24,7 @@ use std::str;
 use std::collections::HashSet;
 use std::iter::FromIterator;
 
-use getopts::Options;
+use getopts::{Options, Matches};
 use rustc_serialize::json::Json;
 
 fn main() {
@@ -70,11 +70,7 @@ fn execute() -> i32 {
         return success;
     }
 
-    let workspace_hitlist = match (matches.opt_present("all"), matches.opt_present("p")) {
-        (false, false) => WorkspaceHitlist::None,
-        (true, _) => WorkspaceHitlist::All,
-        (false, true) => WorkspaceHitlist::Some(matches.opt_strs("p")),
-    };
+    let workspace_hitlist = WorkspaceHitlist::from_matches(&matches);
 
     match format_crate(verbosity, workspace_hitlist) {
         Err(e) => {
@@ -154,41 +150,35 @@ pub struct Target {
     kind: TargetKind,
 }
 
-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq)]
 pub enum WorkspaceHitlist {
     All,
     Some(Vec<String>),
     None,
 }
 
 impl WorkspaceHitlist {
-    fn is_all(&self) -> bool {
-        match *self {
-            WorkspaceHitlist::All => true,
-            _ => false,
-        }
-    }
-
-    fn is_none(&self) -> bool {
-        match *self {
-            WorkspaceHitlist::None => true,
-            _ => false,
+    pub fn get_some<'a>(&'a self) -> Option<&'a [String]> {
+        if let &WorkspaceHitlist::Some(ref hitlist) = self {
+            Some(&hitlist)
+        } else {
+            None
         }
     }
 
-    pub fn get_some<'a>(&'a self) -> Option<&'a [String]> {
-        use std::borrow::Borrow;
-        match self {
-            &WorkspaceHitlist::Some(ref hitlist) => Some(hitlist.borrow()),
-            _ => None,
+    pub fn from_matches(matches: &Matches) -> WorkspaceHitlist {
+        match (matches.opt_present("all"), matches.opt_present("p")) {
+            (false, false) => WorkspaceHitlist::None,
+            (true, _) => WorkspaceHitlist::All,
+            (false, true) => WorkspaceHitlist::Some(matches.opt_strs("p")),
         }
     }
 }
 
 // Returns a vector of all compile targets of a crate
 fn get_targets(workspace_hitlist: WorkspaceHitlist) -> Result<Vec<Target>, std::io::Error> {
     let mut targets: Vec<Target> = vec![];
-    if workspace_hitlist.is_none() {
+    if workspace_hitlist == WorkspaceHitlist::None {
         let output = try!(Command::new("cargo").arg("read-manifest").output());
         if output.status.success() {
             // None of the unwraps should fail if output of `cargo read-manifest` is correct
@@ -210,14 +200,14 @@ fn get_targets(workspace_hitlist: WorkspaceHitlist) -> Result<Vec<Target>, std::
     // This happens when cargo-fmt is not used inside a crate or
     // is used inside a workspace.
     // To ensure backward compatability, we only use `cargo metadata` for workspaces.
-    // TODO: How do we make sure we use either metadata or read-manifest
+    // TODO: Is it possible only use metadata or read-manifest
     let output = Command::new("cargo").arg("metadata")
         .arg("--no-deps")
         .output()?;
     if output.status.success() {
         let data = &String::from_utf8(output.stdout).unwrap();
         let json = Json::from_str(data).unwrap();
-        let mut hitlist: HashSet<&String> = if !workspace_hitlist.is_all() {
+        let mut hitlist: HashSet<&String> = if workspace_hitlist != WorkspaceHitlist::All {
             HashSet::from_iter(workspace_hitlist.get_some().unwrap())
         } else {
             HashSet::new() // Unused
@@ -227,24 +217,20 @@ fn get_targets(workspace_hitlist: WorkspaceHitlist) -> Result<Vec<Target>, std::
             .as_array()
             .unwrap()
             .into_iter()
-            .filter(|member| if workspace_hitlist.is_all() {
+            .filter(|member| if workspace_hitlist == WorkspaceHitlist::All {
                         true
                     } else {
                         let member_name = member.find("name")
                             .unwrap()
                             .as_string()
                             .unwrap();
-                        if hitlist.take(&member_name.to_string()).is_some() {
-                            true
-                        } else {
-                            false
-                        }
+                        hitlist.take(&member_name.to_string()).is_some()
                     })
             .collect();
         if hitlist.len() != 0 {
             // Mimick cargo of only outputting one <package> spec.
             return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput,
-                                           format!("Could not find package: {:?}",
+                                           format!("package `{}` is not a member of the workspace",
                                                    hitlist.iter().next().unwrap())));
         }
         for member in members {