Extract more code we'll need to implement known error fixes (#189)

Follow up to #188 in support of #160
We need to be able to properly translate from a fix spec to a fix in the
`known-error` code.
This extracts the additional code we'll need to be able to do so.

Also improves test coverage

Author: rubberduck203

scope/src/shared/models/internal/command.rs

@@ -1,15 +1,29 @@
-use std::path::Path;
-
+use anyhow::Result;
 use derive_builder::Builder;
+use std::path::Path;
 
-use super::extract_command_path;
+use super::{extract_command_path, substitute_templates};
 
 #[derive(Debug, PartialEq, Clone, Builder)]
 #[builder(setter(into))]
 pub struct DoctorCommand {
     pub commands: Vec<String>,
 }
 
+impl DoctorCommand {
+    pub fn from_commands(
+        containing_dir: &Path,
+        working_dir: &str,
+        commands: &Vec<String>,
+    ) -> Result<DoctorCommand> {
+        let mut templated_commands = Vec::new();
+        for command in commands {
+            templated_commands.push(substitute_templates(working_dir, command)?);
+        }
+        Ok(DoctorCommand::from((containing_dir, templated_commands)))
+    }
+}
+
 impl<T> From<(&Path, Vec<T>)> for DoctorCommand
 where
     String: for<'a> From<&'a T>,
@@ -74,4 +88,26 @@ mod tests {
             actual
         )
     }
+
+    #[test]
+    fn from_commands_succeeds() {
+        let containing_dir = Path::new("/foo/bar");
+        let working_dir = "/some/working_dir";
+        let commands = vec!["{{ working_dir }}/foo.sh", "./bar.sh"]
+            .iter()
+            .map(|cmd| cmd.to_string())
+            .collect::<Vec<String>>();
+
+        let actual = DoctorCommand::from_commands(containing_dir, working_dir, &commands)
+            .expect("Expected Ok");
+
+        let templated_commands = commands
+            .iter()
+            .map(|cmd| substitute_templates(&working_dir, &cmd).unwrap())
+            .collect::<Vec<String>>();
+
+        let expected = DoctorCommand::from((containing_dir, templated_commands));
+
+        assert_eq!(expected, actual)
+    }
 }

scope/src/shared/models/internal/doctor_group.rs

@@ -3,12 +3,13 @@ use std::path::{Path, PathBuf};
 
 use anyhow::Result;
 use derive_builder::Builder;
-use minijinja::{context, Environment};
 
 use crate::models::prelude::{ModelMetadata, V1AlphaDoctorGroup};
 use crate::models::HelpMetadata;
 use crate::prelude::{DoctorGroupActionSpec, DoctorInclude};
-use crate::shared::models::internal::{DoctorCommand, DoctorFix, DoctorFixPrompt};
+use crate::shared::models::internal::{DoctorCommand, DoctorFix};
+
+use super::substitute_templates;
 
 #[derive(Debug, PartialEq, Clone, Builder)]
 #[builder(setter(into))]
@@ -67,15 +68,6 @@ impl HelpMetadata for DoctorGroup {
     }
 }
 
-fn substitute_templates(work_dir: &str, input_str: &str) -> Result<String> {
-    let mut env = Environment::new();
-    env.add_template("input_str", input_str)?;
-    let template = env.get_template("input_str")?;
-    let result = template.render(context! { working_dir => work_dir })?;
-
-    Ok(result)
-}
-
 impl TryFrom<V1AlphaDoctorGroup> for DoctorGroup {
     type Error = anyhow::Error;
 
@@ -112,29 +104,19 @@ fn parse_action(
         .clone();
 
     let spec_action = action.clone();
-    let help_text = spec_action
-        .fix
-        .as_ref()
-        .and_then(|x| x.help_text.as_ref().map(|st| st.trim().to_string()).clone());
-    let help_url = spec_action.fix.as_ref().and_then(|x| x.help_url.clone());
-    let fix_command = if let Some(fix) = &spec_action.fix {
-        let mut templated_commands = Vec::new();
-        for command in &fix.commands {
-            templated_commands.push(substitute_templates(&working_dir, command)?);
-        }
-        Some(DoctorCommand::from((containing_dir, templated_commands)))
-    } else {
-        None
+
+    let fix = match &spec_action.fix {
+        Some(fix_spec) => DoctorFix::from_spec(containing_dir, &working_dir, fix_spec.clone())?,
+        None => DoctorFix::empty(),
     };
 
-    let check_command = if let Some(ref check) = spec_action.check.commands {
-        let mut templated_commands = Vec::new();
-        for command in check {
-            templated_commands.push(substitute_templates(&working_dir, command)?);
-        }
-        Some(DoctorCommand::from((containing_dir, templated_commands)))
-    } else {
-        None
+    let check_command = match spec_action.check.commands {
+        Some(ref check) => Some(DoctorCommand::from_commands(
+            containing_dir,
+            &working_dir,
+            check,
+        )?),
+        None => None,
     };
 
     Ok(DoctorGroupAction {
@@ -143,15 +125,7 @@ fn parse_action(
         description: spec_action
             .description
             .unwrap_or_else(|| "default".to_string()),
-        fix: DoctorFix {
-            command: fix_command,
-            prompt: spec_action
-                .fix
-                .and_then(|fix| fix.prompt)
-                .map(DoctorFixPrompt::from),
-            help_text,
-            help_url,
-        },
+        fix,
         check: DoctorGroupActionCheck {
             command: check_command,
             files: spec_action.check.paths.map(|paths| DoctorGroupCachePath {

scope/src/shared/models/internal/fix.rs

@@ -1,4 +1,10 @@
-use crate::{prelude::DoctorFixPromptSpec, shared::prelude::*};
+use anyhow::Result;
+use std::path::Path;
+
+use crate::{
+    prelude::{DoctorFixPromptSpec, DoctorFixSpec},
+    shared::prelude::*,
+};
 use derive_builder::Builder;
 
 #[derive(Debug, PartialEq, Clone, Builder)]
@@ -14,6 +20,35 @@ pub struct DoctorFix {
     pub prompt: Option<DoctorFixPrompt>,
 }
 
+impl DoctorFix {
+    pub fn empty() -> Self {
+        DoctorFix {
+            command: None,
+            help_text: None,
+            help_url: None,
+            prompt: None,
+        }
+    }
+
+    pub fn from_spec(containing_dir: &Path, working_dir: &str, fix: DoctorFixSpec) -> Result<Self> {
+        let commands = DoctorCommand::from_commands(containing_dir, working_dir, &fix.commands)?;
+        let help_text = fix
+            .help_text
+            .as_ref()
+            .map(|st| st.trim().to_string())
+            .clone();
+        let help_url = fix.help_url.clone();
+        let prompt = fix.prompt.map(DoctorFixPrompt::from);
+
+        Ok(DoctorFix {
+            command: Some(commands),
+            help_text,
+            help_url,
+            prompt,
+        })
+    }
+}
+
 #[derive(Debug, PartialEq, Clone, Builder)]
 #[builder(setter(into))]
 pub struct DoctorFixPrompt {
@@ -53,4 +88,57 @@ mod tests {
             actual
         )
     }
+
+    #[test]
+    fn empty_returns_a_fix_full_of_none() {
+        // I can argue that we should use Option<DoctorFix> instead,
+        // but for now, this is where we're at.
+        assert_eq!(
+            DoctorFix {
+                command: None,
+                help_text: None,
+                help_url: None,
+                prompt: None,
+            },
+            DoctorFix::empty()
+        )
+    }
+
+    #[test]
+    fn from_spec_translates_to_fix() {
+        let spec = DoctorFixSpec {
+            commands: vec![
+                "some/command",
+                "./other_command",
+                "{{ working_dir }}/.foo.sh",
+            ]
+            .iter()
+            .map(|cmd| cmd.to_string())
+            .collect(),
+            help_text: Some("text".to_string()),
+            help_url: Some("https.example.com".to_string()),
+            prompt: Some(DoctorFixPromptSpec {
+                text: "do you want to do the thing?".to_string(),
+                extra_context: Some("additional context".to_string()),
+            }),
+        };
+
+        let expected = DoctorFix {
+            command: Some(
+                DoctorCommand::from_commands(
+                    Path::new("/some/dir"),
+                    "/some/work/dir",
+                    &spec.commands,
+                )
+                .unwrap(),
+            ),
+            help_text: spec.help_text.clone(),
+            help_url: spec.help_url.clone(),
+            prompt: Some(DoctorFixPrompt::from(spec.prompt.clone().unwrap())),
+        };
+
+        let actual = DoctorFix::from_spec(Path::new("/some/dir"), "/some/work/dir", spec).unwrap();
+
+        assert_eq!(expected, actual)
+    }
 }

scope/src/shared/models/internal/mod.rs

@@ -4,6 +4,8 @@ use crate::models::prelude::{
 use crate::models::InternalScopeModel;
 use crate::shared::prelude::*;
 use anyhow::anyhow;
+use anyhow::Result;
+use minijinja::{context, Environment};
 use path_clean::PathClean;
 use serde_yaml::Value;
 use std::collections::VecDeque;
@@ -91,6 +93,15 @@ pub(crate) fn extract_command_path(parent_dir: &Path, exec: &str) -> String {
         .join(" ")
 }
 
+pub(crate) fn substitute_templates(work_dir: &str, input_str: &str) -> Result<String> {
+    let mut env = Environment::new();
+    env.add_template("input_str", input_str)?;
+    let template = env.get_template("input_str")?;
+    let result = template.render(context! { working_dir => work_dir })?;
+
+    Ok(result)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -109,4 +120,40 @@ mod tests {
         assert_eq!("foo", extract_command_path(base_path, "foo"));
         assert_eq!("foo bar", extract_command_path(base_path, "foo bar"));
     }
+
+    mod substitute_templates_spec {
+        use super::*;
+
+        #[test]
+        fn working_dir_is_subbed() {
+            let working_dir = "/some/path";
+            let command = "{{ working_dir }}/foo.sh";
+
+            let actual = substitute_templates(&working_dir, &command).unwrap();
+
+            assert_eq!("/some/path/foo.sh".to_string(), actual)
+        }
+
+        #[test]
+        fn text_without_templates_is_passed_through() {
+            let working_dir = "/some/path";
+            let command = "./foo.sh";
+
+            let actual = substitute_templates(&working_dir, &command).unwrap();
+
+            assert_eq!("./foo.sh".to_string(), actual)
+        }
+
+        #[test]
+        fn other_templates_are_erased() {
+            // I don't believe this is intentional behavior,
+            // but it is the current behavior.
+            let working_dir = "/some/path";
+            let command = "{{ not_a_thing }}/foo.sh";
+
+            let actual = substitute_templates(&working_dir, &command).unwrap();
+
+            assert_eq!("/foo.sh".to_string(), actual)
+        }
+    }
 }