Repository: iqlusioninc/abscissa (language: rust)

Issue:
`EntryPoint` and config paths
I ran into a problem trying to load a config from a file in the following way: I implemented the `config_path` function in `impl Configurable<FooConfig> for FooCommand` to supply the correct config path, and expected the application initialization to call it in order to load the config file.  However, the default boilerplate generates an
```
impl Application for FooApplication {
    type Cmd = EntryPoint<FooCommand>;
```
so that the `command` object created here https://github.com/iqlusioninc/abscissa/blob/977b548286266923a9a1a34c17355870d12b0605/src/application.rs#L54 has type `command: EntryPoint<FooCommand>`, and so when it's configured here https://github.com/iqlusioninc/abscissa/blob/977b548286266923a9a1a34c17355870d12b0605/src/application.rs#L101-L103 the `config_path` function that's called is
```
<abscissa::command::entrypoint::EntryPoint<Cmd> as abscissa::config::configurable::Configurable<Cfg>>::config_path
```
(in fact the first `config_path` result is discarded, but the same function is called again inside of `load_config`).

But since the `EntryPoint` is created using `from_args`, there's no opportunity to actually set its config path, and the `config_path` function for the inner command is never called.

So I'm
a) not sure how to actually set the config path "correctly";
b) a little unclear on what the role of the `EntryPoint` struct is -- why not have `type Cmd = FooCommand`?.


Discussion / hints:
`EntryPoint` not delegating to its `Cmd` is a definite bug, and unfortunately one not presently exercised by the existing boilerplate / tests.

Will fix ASAP.

Fix patch:
diff --git a/abscissa_generator/template/src/commands.rs.hbs b/abscissa_generator/template/src/commands.rs.hbs
index e0f0eea8..08df0b05 100644
--- a/abscissa_generator/template/src/commands.rs.hbs
+++ b/abscissa_generator/template/src/commands.rs.hbs
@@ -15,7 +15,7 @@ mod version;
 
 use self::{start::StartCommand, version::VersionCommand};
 use crate::config::{{~config_type~}};
-use abscissa::{Command, Configurable, Help, Options, Runnable};
+use abscissa::{config::Override, Command, Configurable, FrameworkError, Help, Options, Runnable};
 use std::path::PathBuf;
 
 /// {{title}} Subcommands
@@ -36,9 +36,23 @@ pub enum {{command_type}} {
 
 /// This trait allows you to define how application configuration is loaded.
 impl Configurable<{{~config_type~}}> for {{command_type}} {
+    // Have `config_path` return `Some(path)` in order to trigger the
+    // application configuration being loaded.
     fn config_path(&self) -> Option<PathBuf> {
-        // Have `config_path` return `Some(path)` in order to trigger the
-        // application configuration being loaded.
         None
     }
+
+    // Apply changes to the config after it's been loaded, e.g. overriding
+    // values in a config file using command-line options.
+    //
+    // This can be safely deleted if you don't intend to use it.
+    fn process_config(
+        &self,
+        config: {{config_type}},
+    ) -> Result<{{~config_type~}}, FrameworkError> {
+        match self {
+            {{command_type}}::Start(cmd) => cmd.override_config(config),
+            _ => Ok(config),
+        }
+    }
 }
diff --git a/abscissa_generator/template/src/commands/start.rs.hbs b/abscissa_generator/template/src/commands/start.rs.hbs
index 7d1b8b7d..8fef47d2 100644
--- a/abscissa_generator/template/src/commands/start.rs.hbs
+++ b/abscissa_generator/template/src/commands/start.rs.hbs
@@ -5,7 +5,8 @@
 #[allow(unused_imports)]
 use crate::prelude::*;
 
-use abscissa::{Command, Options, Runnable};
+use crate::config::{{~config_type~}};
+use abscissa::{config, Command, FrameworkError, Options, Runnable};
 
 /// `start` subcommand
 ///
@@ -31,3 +32,19 @@ impl Runnable for StartCommand {
         }
     }
 }
+
+impl config::Override<{{~config_type~}}> for StartCommand {
+    // Process the given command line options, overriding settings from
+    // a configuration file using explicit flags taken from command-line
+    // arguments.
+    fn override_config(
+        &self,
+        mut config: {{config_type}},
+    ) -> Result<{{config_type}}, FrameworkError> {
+        if !self.recipient.is_empty() {
+            config.example_section.example_value = self.recipient.join(" ");
+        }
+
+        Ok(config)
+    }
+}
diff --git a/src/application.rs b/src/application.rs
index 2776d85a..118693a6 100644
--- a/src/application.rs
+++ b/src/application.rs
@@ -21,7 +21,7 @@ use crate::{
     terminal::{component::TerminalComponent, ColorChoice},
     Version,
 };
-use std::{env, process, vec};
+use std::{env, path::Path, process, vec};
 
 /// Application types implementing this trait own global application state,
 /// including configuration and arbitrary other values stored within
@@ -100,7 +100,7 @@ pub trait Application: Default + Sized {
         // Load configuration
         let config = command
             .config_path()
-            .map(|_| self.load_config(command))
+            .map(|path| command.process_config(self.load_config(&path)?))
             .transpose()?;
 
         // Fire callback regardless of whether any config was loaded to
@@ -123,28 +123,21 @@ pub trait Application: Default + Sized {
         Ok(vec![Box::new(terminal), Box::new(logging)])
     }
 
-    /// Load configuration from command's `config_path()`.
+    /// Load configuration from the given path.
     ///
-    /// Returns an error if the configuration could not be loaded or if the
-    /// command's `config_path()` is none.
-    fn load_config(&mut self, command: &Self::Cmd) -> Result<Self::Cfg, FrameworkError> {
-        // Only attempt to load configuration if `config_path` returned the
-        // path to a configuration file.
-        if let Some(ref path) = command.config_path() {
-            let canonical_path = AbsPathBuf::canonicalize(path)
-                .map_err(|e| err!(ConfigError, "invalid path '{}': {}", path.display(), e))?;
-
-            command.load_config_file(&canonical_path).map_err(|e| {
-                err!(
-                    ConfigError,
-                    "error loading config from '{}': {}",
-                    canonical_path.display(),
-                    e
-                )
-            })
-        } else {
-            fail!(PathError, "no config path for command: {:?}", command);
-        }
+    /// Returns an error if the configuration could not be loaded.
+    fn load_config(&mut self, path: &Path) -> Result<Self::Cfg, FrameworkError> {
+        let canonical_path = AbsPathBuf::canonicalize(path)
+            .map_err(|e| err!(ConfigError, "invalid path '{}': {}", path.display(), e))?;
+
+        Self::Cfg::load_toml_file(&canonical_path).map_err(|e| {
+            err!(
+                ConfigError,
+                "error loading config from '{}': {}",
+                canonical_path.display(),
+                e
+            )
+        })
     }
 
     /// Name of this application as a string.
diff --git a/src/command/entrypoint.rs b/src/command/entrypoint.rs
index c3c2cd0a..8df68af9 100644
--- a/src/command/entrypoint.rs
+++ b/src/command/entrypoint.rs
@@ -1,14 +1,17 @@
 //! Toplevel entrypoint command.
 
 use super::{Command, Usage};
-use crate::{Config, Configurable, Options, Runnable};
+use crate::{Config, Configurable, FrameworkError, Options, Runnable};
 use std::path::PathBuf;
 
 /// Toplevel entrypoint command.
 ///
 /// Handles obtaining toplevel help as well as verbosity settings.
 #[derive(Debug, Options)]
-pub struct EntryPoint<Cmd: Runnable + Command> {
+pub struct EntryPoint<Cmd>
+where
+    Cmd: Command + Runnable,
+{
     /// Path to the configuration file
     #[options(help = "path to configuration file")]
     pub config: Option<PathBuf>,
@@ -31,7 +34,7 @@ pub struct EntryPoint<Cmd: Runnable + Command> {
 
 impl<Cmd> EntryPoint<Cmd>
 where
-    Cmd: Runnable + Command,
+    Cmd: Command + Runnable,
 {
     /// Borrow the underlying command type or print usage info and exit
     fn command(&self) -> &Cmd {
@@ -43,7 +46,7 @@ where
 
 impl<Cmd> Runnable for EntryPoint<Cmd>
 where
-    Cmd: Runnable + Command,
+    Cmd: Command + Runnable,
 {
     fn run(&self) {
         self.command().run()
@@ -52,7 +55,7 @@ where
 
 impl<Cmd> Command for EntryPoint<Cmd>
 where
-    Cmd: Runnable + Command,
+    Cmd: Command + Runnable,
 {
     /// Name of this program as a string
     fn name() -> &'static str {
@@ -82,11 +85,26 @@ where
 
 impl<Cfg, Cmd> Configurable<Cfg> for EntryPoint<Cmd>
 where
-    Cmd: Runnable + Command,
+    Cmd: Command + Configurable<Cfg> + Runnable,
     Cfg: Config,
 {
     /// Path to the command's configuration file
     fn config_path(&self) -> Option<PathBuf> {
-        self.config.clone()
+        match &self.config {
+            // Use explicit `-c`/`--config` argument if passed
+            Some(cfg) => Some(cfg.clone()),
+
+            // Otherwise defer to the toplevel command's config path logic
+            None => self.command.as_ref().and_then(|cmd| cmd.config_path()),
+        }
+    }
+
+    /// Process the configuration after it has been loaded, potentially
+    /// modifying it or returning an error if options are incompatible
+    fn process_config(&self, config: Cfg) -> Result<Cfg, FrameworkError> {
+        match &self.command {
+            Some(cmd) => cmd.process_config(config),
+            None => Ok(config),
+        }
     }
 }
diff --git a/src/config.rs b/src/config.rs
index 86cb8809..49f37662 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -1,10 +1,10 @@
 //! Support for managing global configuration, as well as loading it from TOML
 
 mod configurable;
-mod merge;
+mod overrides;
 mod reader;
 
-pub use self::{configurable::Configurable, merge::MergeOptions, reader::Reader};
+pub use self::{configurable::Configurable, overrides::Override, reader::Reader};
 use crate::{
     error::{FrameworkError, FrameworkErrorKind::ConfigError},
     path::AbsPath,
diff --git a/src/config/configurable.rs b/src/config/configurable.rs
index 74f93912..b67bbf12 100644
--- a/src/config/configurable.rs
+++ b/src/config/configurable.rs
@@ -1,25 +1,19 @@
 //! Configuration loader
 
 use super::Config;
-use crate::{error::FrameworkError, path::AbsPath};
+use crate::error::FrameworkError;
 use std::path::PathBuf;
 
 /// Command type with which a configuration file is associated
-pub trait Configurable<C: Config> {
+pub trait Configurable<Cfg: Config> {
     /// Path to the command's configuration file. Returns an error by default.
     fn config_path(&self) -> Option<PathBuf> {
         None
     }
 
-    /// Load the configuration for this command
-    fn load_config_file<P: AsRef<AbsPath>>(&self, path: &P) -> Result<C, FrameworkError> {
-        self.preprocess_config(C::load_toml_file(path)?)
-    }
-
     /// Process the configuration after it has been loaded, potentially
     /// modifying it or returning an error if options are incompatible
-    #[allow(unused_mut)]
-    fn preprocess_config(&self, mut config: C) -> Result<C, FrameworkError> {
+    fn process_config(&self, config: Cfg) -> Result<Cfg, FrameworkError> {
         Ok(config)
     }
 }
diff --git a/src/config/merge.rs b/src/config/overrides.rs
similarity index 50%
rename from src/config/merge.rs
rename to src/config/overrides.rs
index 75fb7ced..9d4e2454 100644
--- a/src/config/merge.rs
+++ b/src/config/overrides.rs
@@ -1,13 +1,9 @@
-//! Support for sourcing/overriding configuration values from arguments
-//! given on the command-line.
+//! Override values in the configuration file with command-line options
 
-use crate::Options;
+use crate::{Command, Config, FrameworkError};
 
-/// Merge the given options into this configuration. This allows setting of
-/// global configuration values using command-line options, and also unifies
-/// the global config as the one way to get application settings.
-#[allow(unused_variables)]
-pub trait MergeOptions<O: Options>: Sized {
+/// Use options from the given `Command` to override settings in the config.
+pub trait Override<Cfg: Config>: Command {
     /// Process the given command line options, overriding settings from
     /// a configuration file using explicit flags taken from command-line
     /// arguments.
@@ -16,7 +12,7 @@ pub trait MergeOptions<O: Options>: Sized {
     /// settings when dealing with both a config file and options passed
     /// on the command line, and a unified way of accessing this information
     /// from components or in the application: from the global config.
-    fn merge(self, options: &O) -> Self {
-        self
+    fn override_config(&self, config: Cfg) -> Result<Cfg, FrameworkError> {
+        Ok(config)
     }
 }


Test patch: