From 59e889c6786325c786153f73ee0126cb6ea8f8bf Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 10:19:04 +0200 Subject: [PATCH 01/15] Add `ModelPath` --- crates/fj-app/src/main.rs | 27 +++++++------------- crates/fj-app/src/path.rs | 53 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 18 deletions(-) create mode 100644 crates/fj-app/src/path.rs diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 423862147..76439cdc6 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -14,14 +14,14 @@ mod args; mod config; +mod path; -use std::path::PathBuf; - -use anyhow::{anyhow, Context as _}; +use anyhow::Context as _; use fj_export::export; use fj_host::{Model, Parameters}; use fj_operations::shape_processor::ShapeProcessor; use fj_window::run::run; +use path::ModelPath; use tracing_subscriber::fmt::format; use tracing_subscriber::EnvFilter; @@ -43,36 +43,27 @@ fn main() -> anyhow::Result<()> { let args = Args::parse(); let config = Config::load()?; - let path = config.default_path.unwrap_or_else(|| PathBuf::from("")); + let path = ModelPath::from_args_and_config(&args, &config)?; let parameters = args.parameters.unwrap_or_else(Parameters::empty); let shape_processor = ShapeProcessor { tolerance: args.tolerance, }; - let path_of_model = path.canonicalize().unwrap_or_default(); + let path_of_model = path.default_path().canonicalize().unwrap_or_default(); - let model = if let Some(model) = - args.model.or(config.default_model).as_ref() - { - let mut model_path = path; - model_path.push(model); + let model = { + let model_path = path.path(); Model::new(model_path.clone(), parameters).with_context(|| { if path_of_model.as_os_str().is_empty() { format!( - "Model is not defined, can't find model defined inside the default-model also, add model like \n cargo run -- -m {}", model.display() + "Model is not defined, can't find model defined inside the default-model also, add model like \n cargo run -- -m {}", path.model_path_without_default().display() ) } else { format!( - "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", model_path.display(), path_of_model.display(), model.display() + "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", model_path.display(), path_of_model.display(), path.model_path_without_default().display() ) } })? - } else { - return Err(anyhow!( - "You must specify a model to start Fornjot.\n\ - - Pass a model as a command-line argument. See `fj-app --help`.\n\ - - Specify a default model in the configuration file." - )); }; if let Some(export_path) = args.export { diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs new file mode 100644 index 000000000..78b2c09f2 --- /dev/null +++ b/crates/fj-app/src/path.rs @@ -0,0 +1,53 @@ +use std::path::{Path, PathBuf}; + +use anyhow::anyhow; + +use crate::{args::Args, config::Config}; + +pub struct ModelPath { + default_path: PathBuf, + model_path: PathBuf, +} + +impl ModelPath { + pub fn from_args_and_config( + args: &Args, + config: &Config, + ) -> anyhow::Result { + let default_path = config + .default_path + .clone() + .unwrap_or_else(|| PathBuf::from("")); + let model_path = args + .model + .as_ref() + .or(config.default_model.as_ref()) + .ok_or_else(no_model_error)? + .clone(); + + Ok(Self { + default_path, + model_path, + }) + } + + pub fn default_path(&self) -> PathBuf { + self.default_path.clone() + } + + pub fn model_path_without_default(&self) -> &Path { + &self.model_path + } + + pub fn path(&self) -> PathBuf { + self.default_path.join(&self.model_path) + } +} + +fn no_model_error() -> anyhow::Error { + anyhow!( + "You must specify a model to start Fornjot.\n\ + - Pass a model as a command-line argument. See `fj-app --help`.\n\ + - Specify a default model in the configuration file." + ) +} From 8b5026af2fb4fdcfaf4d9c19f9af86f2d94a94bd Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 10:34:27 +0200 Subject: [PATCH 02/15] Handle missing `default-path` in `ModelPath` --- crates/fj-app/src/main.rs | 12 ++---------- crates/fj-app/src/path.rs | 10 +++++++++- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 76439cdc6..0aaf839f6 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -49,20 +49,12 @@ fn main() -> anyhow::Result<()> { tolerance: args.tolerance, }; - let path_of_model = path.default_path().canonicalize().unwrap_or_default(); - let model = { let model_path = path.path(); Model::new(model_path.clone(), parameters).with_context(|| { - if path_of_model.as_os_str().is_empty() { - format!( - "Model is not defined, can't find model defined inside the default-model also, add model like \n cargo run -- -m {}", path.model_path_without_default().display() - ) - } else { - format!( - "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", model_path.display(), path_of_model.display(), path.model_path_without_default().display() + format!( + "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", model_path.display(), path.default_path().display(), path.model_path_without_default().display() ) - } })? }; diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index 78b2c09f2..6d17282b2 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -1,6 +1,6 @@ use std::path::{Path, PathBuf}; -use anyhow::anyhow; +use anyhow::{anyhow, Context}; use crate::{args::Args, config::Config}; @@ -18,6 +18,14 @@ impl ModelPath { .default_path .clone() .unwrap_or_else(|| PathBuf::from("")); + let default_path = default_path.canonicalize().with_context(|| { + format!( + "Converting `default-path` from `fj.toml` (`{}`) into absolute \ + path", + default_path.display(), + ) + })?; + let model_path = args .model .as_ref() From 13fc7b48c1260d01f84acc4d0d46e3888dad036d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 10:43:26 +0200 Subject: [PATCH 03/15] Remember whether `default-path` was specified This makes things a bit more complicated for now, but it will help build a better error context in the next step. --- crates/fj-app/src/path.rs | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index 6d17282b2..c2c11bed2 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -5,7 +5,7 @@ use anyhow::{anyhow, Context}; use crate::{args::Args, config::Config}; pub struct ModelPath { - default_path: PathBuf, + default_path: Option, model_path: PathBuf, } @@ -17,14 +17,16 @@ impl ModelPath { let default_path = config .default_path .clone() - .unwrap_or_else(|| PathBuf::from("")); - let default_path = default_path.canonicalize().with_context(|| { - format!( - "Converting `default-path` from `fj.toml` (`{}`) into absolute \ - path", - default_path.display(), - ) - })?; + .map(|path| { + path.canonicalize().with_context(|| { + format!( + "Converting `default-path` from `fj.toml` (`{}`) into \ + absolute path", + path.display(), + ) + }) + }) + .transpose()?; let model_path = args .model @@ -40,7 +42,7 @@ impl ModelPath { } pub fn default_path(&self) -> PathBuf { - self.default_path.clone() + self.default_path.clone().unwrap_or_else(PathBuf::new) } pub fn model_path_without_default(&self) -> &Path { @@ -48,7 +50,10 @@ impl ModelPath { } pub fn path(&self) -> PathBuf { - self.default_path.join(&self.model_path) + self.default_path + .clone() + .unwrap_or_else(PathBuf::new) + .join(&self.model_path) } } From 7f9635c4cc1fa6dd6e49aed6459e3456a1064400 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 24 Oct 2022 13:48:49 +0200 Subject: [PATCH 04/15] Don't require `PathBuf` in `Model::new` --- crates/fj-host/src/model.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index a27039018..7bc3d5e94 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -22,7 +22,12 @@ impl Model { /// /// The path expected here is the root directory of the model's Cargo /// package, that is the folder containing `Cargo.toml`. - pub fn new(path: PathBuf, parameters: Parameters) -> Result { + pub fn new( + path: impl AsRef, + parameters: Parameters, + ) -> Result { + let path = path.as_ref(); + let crate_dir = path.canonicalize()?; let metadata = cargo_metadata::MetadataCommand::new() From fc8ba771f24450c3a2f0abcddae9408fd3a62d30 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 10:47:31 +0200 Subject: [PATCH 05/15] Add `ModelPath::load_model` --- crates/fj-app/src/main.rs | 4 ++-- crates/fj-app/src/path.rs | 7 +++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 0aaf839f6..70d302422 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -18,7 +18,7 @@ mod path; use anyhow::Context as _; use fj_export::export; -use fj_host::{Model, Parameters}; +use fj_host::Parameters; use fj_operations::shape_processor::ShapeProcessor; use fj_window::run::run; use path::ModelPath; @@ -51,7 +51,7 @@ fn main() -> anyhow::Result<()> { let model = { let model_path = path.path(); - Model::new(model_path.clone(), parameters).with_context(|| { + path.load_model(parameters).with_context(|| { format!( "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", model_path.display(), path.default_path().display(), path.model_path_without_default().display() ) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index c2c11bed2..56490f242 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -1,6 +1,7 @@ use std::path::{Path, PathBuf}; use anyhow::{anyhow, Context}; +use fj_host::{Model, Parameters}; use crate::{args::Args, config::Config}; @@ -55,6 +56,12 @@ impl ModelPath { .unwrap_or_else(PathBuf::new) .join(&self.model_path) } + + pub fn load_model(&self, parameters: Parameters) -> anyhow::Result { + let path = self.path(); + let model = Model::new(&path, parameters)?; + Ok(model) + } } fn no_model_error() -> anyhow::Error { From 8dfbe08dcb80e3dbbaffab9a99385fcd7bd0b266 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 10:48:37 +0200 Subject: [PATCH 06/15] Inline variable --- crates/fj-app/src/main.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 70d302422..bb7e4bc06 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -50,10 +50,9 @@ fn main() -> anyhow::Result<()> { }; let model = { - let model_path = path.path(); path.load_model(parameters).with_context(|| { format!( - "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", model_path.display(), path.default_path().display(), path.model_path_without_default().display() + "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", path.path().display(), path.default_path().display(), path.model_path_without_default().display() ) })? }; From 25558e8798a9b8f78d87ff80adf762698ad7d513 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 10:48:53 +0200 Subject: [PATCH 07/15] Make variable name more specific --- crates/fj-app/src/main.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index bb7e4bc06..7bb1b7727 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -43,16 +43,16 @@ fn main() -> anyhow::Result<()> { let args = Args::parse(); let config = Config::load()?; - let path = ModelPath::from_args_and_config(&args, &config)?; + let model_path = ModelPath::from_args_and_config(&args, &config)?; let parameters = args.parameters.unwrap_or_else(Parameters::empty); let shape_processor = ShapeProcessor { tolerance: args.tolerance, }; let model = { - path.load_model(parameters).with_context(|| { + model_path.load_model(parameters).with_context(|| { format!( - "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", path.path().display(), path.default_path().display(), path.model_path_without_default().display() + "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", model_path.path().display(), model_path.default_path().display(), model_path.model_path_without_default().display() ) })? }; From c9f3b7e7ab957eff63bb7a98c7991a4ce24ca0dc Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 11:17:07 +0200 Subject: [PATCH 08/15] Build error context inside `load_path` --- crates/fj-app/src/main.rs | 9 +------- crates/fj-app/src/path.rs | 44 ++++++++++++++++++++++++++++++--------- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 7bb1b7727..a07e1e467 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -16,7 +16,6 @@ mod args; mod config; mod path; -use anyhow::Context as _; use fj_export::export; use fj_host::Parameters; use fj_operations::shape_processor::ShapeProcessor; @@ -49,13 +48,7 @@ fn main() -> anyhow::Result<()> { tolerance: args.tolerance, }; - let model = { - model_path.load_model(parameters).with_context(|| { - format!( - "Failed to load model: {0}\ninside default models directory: '{1}'\nCan mainly caused by: \n1. Model '{2}' can not be found inside '{1}'\n2.'{2}' can be mis-typed see inside '{1}' for a match\n3. Define model is '{2}' couldn\'t be found ((defined in command-line arguments))", model_path.path().display(), model_path.default_path().display(), model_path.model_path_without_default().display() - ) - })? - }; + let model = model_path.load_model(parameters)?; if let Some(export_path) = args.export { // export only mode. just load model, process, export and exit diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index 56490f242..f49285416 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::{fmt::Write, path::PathBuf}; use anyhow::{anyhow, Context}; use fj_host::{Model, Parameters}; @@ -42,14 +42,6 @@ impl ModelPath { }) } - pub fn default_path(&self) -> PathBuf { - self.default_path.clone().unwrap_or_else(PathBuf::new) - } - - pub fn model_path_without_default(&self) -> &Path { - &self.model_path - } - pub fn path(&self) -> PathBuf { self.default_path .clone() @@ -59,7 +51,39 @@ impl ModelPath { pub fn load_model(&self, parameters: Parameters) -> anyhow::Result { let path = self.path(); - let model = Model::new(&path, parameters)?; + + let mut error = String::new(); + write!( + error, + "Failed to load model: `{}`", + self.model_path.display() + )?; + write!(error, "\n- Path of model: {}", path.display())?; + + let mut suggestions = String::new(); + write!(suggestions, "Suggestions:")?; + write!( + suggestions, + "\n- Did you mis-type the model path `{}`?", + self.model_path.display() + )?; + + if let Some(default_path) = &self.default_path { + write!( + error, + "\n- Searching inside default path from configuration: {}", + default_path.display(), + )?; + + write!(suggestions, "\n- Did you mis-type the default path?")?; + write!( + suggestions, + "\n- Did you accidentally pick up a local configuration file?" + )?; + } + + let model = Model::new(&path, parameters) + .with_context(|| format!("{error}\n\n{suggestions}"))?; Ok(model) } } From 2dfff88275aad75cd5eb0268e8cfd05c12769fae Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 11:18:12 +0200 Subject: [PATCH 09/15] Inline method --- crates/fj-app/src/path.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index f49285416..1e1059057 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -42,15 +42,12 @@ impl ModelPath { }) } - pub fn path(&self) -> PathBuf { - self.default_path + pub fn load_model(&self, parameters: Parameters) -> anyhow::Result { + let path = self + .default_path .clone() .unwrap_or_else(PathBuf::new) - .join(&self.model_path) - } - - pub fn load_model(&self, parameters: Parameters) -> anyhow::Result { - let path = self.path(); + .join(&self.model_path); let mut error = String::new(); write!( From c34a02d3ec2b7a874c39b75b854d768da12169b0 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 11:20:38 +0200 Subject: [PATCH 10/15] Refactor --- crates/fj-app/src/path.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index 1e1059057..4a5f21498 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -15,20 +15,7 @@ impl ModelPath { args: &Args, config: &Config, ) -> anyhow::Result { - let default_path = config - .default_path - .clone() - .map(|path| { - path.canonicalize().with_context(|| { - format!( - "Converting `default-path` from `fj.toml` (`{}`) into \ - absolute path", - path.display(), - ) - }) - }) - .transpose()?; - + let default_path = config.default_path.clone(); let model_path = args .model .as_ref() @@ -43,8 +30,21 @@ impl ModelPath { } pub fn load_model(&self, parameters: Parameters) -> anyhow::Result { - let path = self + let default_path = self .default_path + .as_ref() + .map(|path| { + path.canonicalize().with_context(|| { + format!( + "Converting `default-path` from `fj.toml` (`{}`) into \ + absolute path", + path.display(), + ) + }) + }) + .transpose()?; + + let path = default_path .clone() .unwrap_or_else(PathBuf::new) .join(&self.model_path); @@ -65,7 +65,7 @@ impl ModelPath { self.model_path.display() )?; - if let Some(default_path) = &self.default_path { + if let Some(default_path) = &default_path { write!( error, "\n- Searching inside default path from configuration: {}", From 3409fad910614bb29e587eb12acea427ecf9bca2 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 11:26:42 +0200 Subject: [PATCH 11/15] Don't throw away relative default path --- crates/fj-app/src/path.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index 4a5f21498..e79db178a 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -33,19 +33,22 @@ impl ModelPath { let default_path = self .default_path .as_ref() - .map(|path| { - path.canonicalize().with_context(|| { + .map(|path| -> anyhow::Result<_> { + let rel = path; + let abs = path.canonicalize().with_context(|| { format!( "Converting `default-path` from `fj.toml` (`{}`) into \ absolute path", path.display(), ) - }) + })?; + Ok((rel, abs)) }) .transpose()?; let path = default_path .clone() + .map(|(_, abs)| abs) .unwrap_or_else(PathBuf::new) .join(&self.model_path); @@ -65,11 +68,11 @@ impl ModelPath { self.model_path.display() )?; - if let Some(default_path) = &default_path { + if let Some((_, default_path_abs)) = &default_path { write!( error, "\n- Searching inside default path from configuration: {}", - default_path.display(), + default_path_abs.display(), )?; write!(suggestions, "\n- Did you mis-type the default path?")?; From b783890dc1dabdb6c72a5c99b37871852deff398 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 11:27:31 +0200 Subject: [PATCH 12/15] Improve error message --- crates/fj-app/src/path.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index e79db178a..d1b2cad28 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -68,14 +68,18 @@ impl ModelPath { self.model_path.display() )?; - if let Some((_, default_path_abs)) = &default_path { + if let Some((default_path_rel, default_path_abs)) = &default_path { write!( error, "\n- Searching inside default path from configuration: {}", default_path_abs.display(), )?; - write!(suggestions, "\n- Did you mis-type the default path?")?; + write!( + suggestions, + "\n- Did you mis-type the default path `{}`?", + default_path_rel.display() + )?; write!( suggestions, "\n- Did you accidentally pick up a local configuration file?" From fbb6c2f3c46e4ff6891ad49fa1adfebabb42d186 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 11:36:29 +0200 Subject: [PATCH 13/15] Build error context in dedicated function --- crates/fj-app/src/path.rs | 81 ++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 31 deletions(-) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index d1b2cad28..924fb5c34 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -1,4 +1,7 @@ -use std::{fmt::Write, path::PathBuf}; +use std::{ + fmt::{self, Write}, + path::{Path, PathBuf}, +}; use anyhow::{anyhow, Context}; use fj_host::{Model, Parameters}; @@ -52,44 +55,60 @@ impl ModelPath { .unwrap_or_else(PathBuf::new) .join(&self.model_path); - let mut error = String::new(); + let model = Model::new(&path, parameters).with_context(|| { + load_error_context(default_path, &self.model_path, path) + })?; + Ok(model) + } +} + +fn load_error_context( + default_path: Option<(&PathBuf, PathBuf)>, + model_path: &Path, + path: PathBuf, +) -> String { + load_error_context_inner(default_path, model_path, path) + .expect("Expected `write!` to `String` to never fail") +} + +fn load_error_context_inner( + default_path: Option<(&PathBuf, PathBuf)>, + model_path: &Path, + path: PathBuf, +) -> Result { + let mut error = String::new(); + write!(error, "Failed to load model: `{}`", model_path.display())?; + write!(error, "\n- Path of model: {}", path.display())?; + + let mut suggestions = String::new(); + write!(suggestions, "Suggestions:")?; + write!( + suggestions, + "\n- Did you mis-type the model path `{}`?", + model_path.display() + )?; + + if let Some((default_path_rel, default_path_abs)) = &default_path { write!( error, - "Failed to load model: `{}`", - self.model_path.display() + "\n- Searching inside default path from configuration: {}", + default_path_abs.display(), )?; - write!(error, "\n- Path of model: {}", path.display())?; - let mut suggestions = String::new(); - write!(suggestions, "Suggestions:")?; write!( suggestions, - "\n- Did you mis-type the model path `{}`?", - self.model_path.display() + "\n- Did you mis-type the default path `{}`?", + default_path_rel.display() + )?; + write!( + suggestions, + "\n- Did you accidentally pick up a local configuration file?" )?; - - if let Some((default_path_rel, default_path_abs)) = &default_path { - write!( - error, - "\n- Searching inside default path from configuration: {}", - default_path_abs.display(), - )?; - - write!( - suggestions, - "\n- Did you mis-type the default path `{}`?", - default_path_rel.display() - )?; - write!( - suggestions, - "\n- Did you accidentally pick up a local configuration file?" - )?; - } - - let model = Model::new(&path, parameters) - .with_context(|| format!("{error}\n\n{suggestions}"))?; - Ok(model) } + + let context = format!("{error}\n\n{suggestions}"); + + Ok(context) } fn no_model_error() -> anyhow::Error { From 4b0c1e6c556f27d92af9e0b7b4f57b89062708dd Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 11:44:07 +0200 Subject: [PATCH 14/15] Remember where model path came from --- crates/fj-app/src/path.rs | 44 ++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index 924fb5c34..d07017fd9 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -10,7 +10,7 @@ use crate::{args::Args, config::Config}; pub struct ModelPath { default_path: Option, - model_path: PathBuf, + model_path: ModelPathSource, } impl ModelPath { @@ -19,12 +19,18 @@ impl ModelPath { config: &Config, ) -> anyhow::Result { let default_path = config.default_path.clone(); - let model_path = args + + let model_path_from_args = args .model .as_ref() - .or(config.default_model.as_ref()) - .ok_or_else(no_model_error)? - .clone(); + .map(|model| ModelPathSource::Args(model.clone())); + let model_path_from_config = config + .default_model + .as_ref() + .map(|model| ModelPathSource::Config(model.clone())); + let model_path = model_path_from_args + .or(model_path_from_config) + .ok_or_else(no_model_error)?; Ok(Self { default_path, @@ -53,7 +59,7 @@ impl ModelPath { .clone() .map(|(_, abs)| abs) .unwrap_or_else(PathBuf::new) - .join(&self.model_path); + .join(self.model_path.path()); let model = Model::new(&path, parameters).with_context(|| { load_error_context(default_path, &self.model_path, path) @@ -62,9 +68,23 @@ impl ModelPath { } } +enum ModelPathSource { + Args(PathBuf), + Config(PathBuf), +} + +impl ModelPathSource { + fn path(&self) -> &Path { + match self { + ModelPathSource::Args(path) => path, + ModelPathSource::Config(path) => path, + } + } +} + fn load_error_context( default_path: Option<(&PathBuf, PathBuf)>, - model_path: &Path, + model_path: &ModelPathSource, path: PathBuf, ) -> String { load_error_context_inner(default_path, model_path, path) @@ -73,11 +93,15 @@ fn load_error_context( fn load_error_context_inner( default_path: Option<(&PathBuf, PathBuf)>, - model_path: &Path, + model_path: &ModelPathSource, path: PathBuf, ) -> Result { let mut error = String::new(); - write!(error, "Failed to load model: `{}`", model_path.display())?; + write!( + error, + "Failed to load model: `{}`", + model_path.path().display() + )?; write!(error, "\n- Path of model: {}", path.display())?; let mut suggestions = String::new(); @@ -85,7 +109,7 @@ fn load_error_context_inner( write!( suggestions, "\n- Did you mis-type the model path `{}`?", - model_path.display() + model_path.path().display() )?; if let Some((default_path_rel, default_path_abs)) = &default_path { From 9735a949c04d443157c5195687f248f5efe93dc4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 25 Oct 2022 11:47:31 +0200 Subject: [PATCH 15/15] Improve error message --- crates/fj-app/src/path.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/fj-app/src/path.rs b/crates/fj-app/src/path.rs index d07017fd9..3fd33d361 100644 --- a/crates/fj-app/src/path.rs +++ b/crates/fj-app/src/path.rs @@ -102,6 +102,14 @@ fn load_error_context_inner( "Failed to load model: `{}`", model_path.path().display() )?; + match model_path { + ModelPathSource::Args(_) => { + write!(error, "\n- Passed via command-line argument")? + } + ModelPathSource::Config(_) => { + write!(error, "\n- Specified as default model in configuration")? + } + } write!(error, "\n- Path of model: {}", path.display())?; let mut suggestions = String::new();