feat: add modules support to commands, refactor config parsing

cablehead · cablehead · commit 0bbde3a72e23 · 2025-03-14T10:11:10.000-04:00
- Create CommonOptions for shared configuration between handlers and commands
- Add modules support to commands (previously only in handlers)
- Extract common config parsing into a shared implementation
- Update tests to work with new implementation
diff --git a/.gitignore b/.gitignore
@@ -2,3 +2,4 @@
 store
 .aider*
 .env
+solid-ui
diff --git a/docs/src/content/docs/reference/commands.mdx b/docs/src/content/docs/reference/commands.mdx
@@ -19,6 +19,7 @@ To create a command, append a definition string with the topic
 
 ```nushell
 r#'{
+  # Required: Command closure
   run: {|frame|
     # frame.topic - always <command>.call
     # frame.hash - contains input content if present
@@ -27,6 +28,17 @@ r#'{
     let n = $frame.meta.args.n
     1..($n) | each {$"($in): ($input)"}
   }
+
+  # Optional: Module definitions
+  modules: {
+    "my-util": "export def format [x] { $\"formatted: ($x)\" }"
+  }
+
+  # Optional: Control output frame behavior
+  return_options: {
+    suffix: ".output" # Output topic suffix (default: ".recv")
+    ttl: "head:1" # Keep only most recent frame
+  }
 }'# | .append repeat.define
 ```
 
@@ -75,6 +87,24 @@ If a command encounters an error during execution, it will:
 Unlike generators, commands do not automatically restart on error - each
 invocation is independent.
 
+## Modules
+
+Commands can use custom Nushell modules:
+
+```nushell
+r#'{
+  run: {|frame|
+    my-math double (frame.meta.args.number)
+  }
+
+  modules: {
+    "my-math": "export def double [x] { $x * 2 }"
+  }
+}'# | .append calculator.define
+```
+
+This allows you to modularize your commands and reuse code across different commands.
+
 ## Key Differences
 
 | Feature        | Commands                | Handlers               | Generators      |
@@ -84,3 +114,4 @@ invocation is independent.
 | Results        | Streamed immediately    | Batched on completion  | Streamed        |
 | Parallelism    | Multiple parallel calls | Sequential processing  | Single instance |
 | Error Handling | Per-invocation          | Unregisters handler    | Auto-restarts   |
+| Modules        | Supported               | Supported              | Not supported   |
diff --git a/src/commands/serve.rs b/src/commands/serve.rs
@@ -2,20 +2,11 @@ use scru128::Scru128Id;
 use std::collections::HashMap;
 use tracing::instrument;
 
-use serde::{Deserialize, Serialize};
-
 use crate::error::Error;
 use crate::nu;
 use crate::nu::commands;
-use crate::nu::util::value_to_json;
-use crate::store::{FollowOption, Frame, ReadOptions, Store, TTL};
-
-// TODO: DRY with handlers
-#[derive(Clone, Debug, Serialize, Deserialize, Default)]
-pub struct ReturnOptions {
-    pub suffix: Option<String>,
-    pub ttl: Option<TTL>,
-}
+use crate::nu::ReturnOptions;
+use crate::store::{FollowOption, Frame, ReadOptions, Store};
 
 #[derive(Clone)]
 struct Command {
@@ -127,14 +118,14 @@ async fn register_command(
         )),
     ])?;
 
-    // Parse the command configuration to extract return_options (ignore the run closure here)
-    let (_closure, return_options) = parse_command_definition(&mut engine, &definition)?;
+    // Parse the command configuration
+    let common_options = nu::parse_config(&mut engine, &definition)?;
 
     Ok(Command {
         id: frame.id,
         engine,
         definition,
-        return_options,
+        return_options: common_options.return_options,
     })
 }
 
@@ -168,10 +159,11 @@ async fn execute_command(command: Command, frame: &Frame, store: &Store) -> Resu
             ),
         )])?;
 
-        let (closure, _) = parse_command_definition(&mut engine, &command.definition)?;
+        // Parse the command configuration to get the up-to-date closure with modules loaded
+        let common_options = nu::parse_config(&mut engine, &command.definition)?;
 
         // Run command and process pipeline
-        match run_command(&engine, closure, &frame) {
+        match run_command(&engine, common_options.run, &frame) {
             Ok(pipeline_data) => {
                 let recv_suffix = command
                     .return_options
@@ -185,7 +177,7 @@ async fn execute_command(command: Command, frame: &Frame, store: &Store) -> Resu
 
                 // Process each value as a .recv event
                 for value in pipeline_data {
-                    let hash = store.cas_insert_sync(value_to_json(&value).to_string())?;
+                    let hash = store.cas_insert_sync(nu::value_to_json(&value).to_string())?;
                     let _ = store.append(
                         Frame::builder(
                             format!(
@@ -264,56 +256,3 @@ fn run_command(
         nu_protocol::PipelineData::empty(),
     )
 }
-
-fn parse_command_definition(
-    engine: &mut nu::Engine,
-    script: &str,
-) -> Result<(nu_protocol::engine::Closure, Option<ReturnOptions>), Error> {
-    let mut working_set = nu_protocol::engine::StateWorkingSet::new(&engine.state);
-    let block = nu_parser::parse(&mut working_set, None, script.as_bytes(), false);
-
-    engine.state.merge_delta(working_set.render())?;
-
-    let mut stack = nu_protocol::engine::Stack::new();
-    let result = nu_engine::eval_block_with_early_return::<nu_protocol::debugger::WithoutDebug>(
-        &engine.state,
-        &mut stack,
-        &block,
-        nu_protocol::PipelineData::empty(),
-    )?;
-
-    let config = result.into_value(nu_protocol::Span::unknown())?;
-
-    // Get the run closure (required)
-    let run = config
-        .get_data_by_key("run")
-        .ok_or("No 'run' field found in command configuration")?
-        .into_closure()?;
-
-    // Optionally parse return_options (using the same approach as in handlers)
-    let return_options = if let Some(return_config) = config.get_data_by_key("return_options") {
-        let record = return_config
-            .as_record()
-            .map_err(|_| "return must be a record")?;
-
-        let suffix = record
-            .get("suffix")
-            .map(|v| v.as_str().map_err(|_| "suffix must be a string"))
-            .transpose()?
-            .map(String::from);
-
-        let ttl = record
-            .get("ttl")
-            .map(|v| serde_json::from_str(&value_to_json(v).to_string()))
-            .transpose()
-            .map_err(|e| format!("invalid TTL: {}", e))?;
-
-        Some(ReturnOptions { suffix, ttl })
-    } else {
-        None
-    };
-
-    engine.state.merge_env(&mut stack)?;
-
-    Ok((run, return_options))
-}
diff --git a/src/handlers/handler.rs b/src/handlers/handler.rs
@@ -1,4 +1,3 @@
-use std::collections::HashMap;
 use std::str::FromStr;
 use std::sync::{Arc, Mutex};
 use std::time::Duration;
@@ -16,14 +15,9 @@ use scru128::Scru128Id;
 use crate::error::Error;
 use crate::nu;
 use crate::nu::commands;
-use crate::nu::util::value_to_json;
-use crate::store::{FollowOption, Frame, ReadOptions, Store, TTL};
-
-#[derive(Clone, Debug, Serialize, Deserialize, Default)]
-pub struct ReturnOptions {
-    pub suffix: Option<String>,
-    pub ttl: Option<TTL>,
-}
+use crate::nu::value_to_json;
+use crate::nu::ReturnOptions;
+use crate::store::{FollowOption, Frame, ReadOptions, Store};
 
 #[derive(Clone)]
 pub struct Handler {
@@ -38,7 +32,6 @@ pub struct Handler {
 #[derive(Clone, Debug)]
 struct HandlerConfig {
     resume_from: ResumeFrom,
-    modules: HashMap<String, String>,
     pulse: Option<u64>,
     return_options: Option<ReturnOptions>,
 }
@@ -82,22 +75,10 @@ impl Handler {
             )),
         ])?;
 
-        let (mut run, mut config) = parse_handler_configuration_script(&mut engine, &expression)?;
-
-        // Load modules and reparse if needed
-        if !config.modules.is_empty() {
-            for (name, content) in &config.modules {
-                tracing::debug!("Loading module '{}'", name);
-                engine
-                    .add_module(name, content)
-                    .map_err(|e| format!("Failed to load module '{}': {}", name, e))?;
-            }
-
-            // we need to re-parse the expression after loading modules, so that the closure has access
-            // to the additional modules: not the best, but I can't see a better way
-            (run, config) = parse_handler_configuration_script(&mut engine, &expression)?;
-        }
+        // Parse configuration (this will handle module loading internally)
+        let (run, config) = parse_handler_configuration_script(&mut engine, &expression)?;
 
+        // Validate the closure signature
         let block = engine.state.get_block(run.block_id);
         if block.signature.required_positional.len() != 1 {
             return Err(format!(
@@ -423,41 +404,17 @@ use nu_protocol::debugger::WithoutDebug;
 use nu_protocol::engine::{Closure, Stack, StateWorkingSet};
 use nu_protocol::PipelineData;
 
-use nu_protocol::format_shell_error;
-use nu_protocol::ShellError;
-
 fn parse_handler_configuration_script(
     engine: &mut nu::Engine,
     script: &str,
 ) -> Result<(Closure, HandlerConfig), Error> {
-    let mut working_set = StateWorkingSet::new(&engine.state);
+    // Parse common options first (includes run, modules, return_options)
+    let common_options = nu::parse_config(engine, script)?;
 
+    // Parse handler-specific options
+    let mut working_set = StateWorkingSet::new(&engine.state);
     let block = parse(&mut working_set, None, script.as_bytes(), false);
 
-    // Handle parse errors
-    if let Some(err) = working_set.parse_errors.first() {
-        let shell_error = ShellError::GenericError {
-            error: "Parse error".into(),
-            msg: format!("{:?}", err),
-            span: Some(err.span()),
-            help: None,
-            inner: vec![],
-        };
-        return Err(Error::from(format_shell_error(&working_set, &shell_error)));
-    }
-
-    // Handle compile errors
-    if let Some(err) = working_set.compile_errors.first() {
-        let shell_error = ShellError::GenericError {
-            error: "Compile error".into(),
-            msg: format!("{:?}", err),
-            span: None,
-            help: None,
-            inner: vec![],
-        };
-        return Err(Error::from(format_shell_error(&working_set, &shell_error)));
-    }
-
     engine.state.merge_delta(working_set.render())?;
 
     let mut stack = Stack::new();
@@ -466,19 +423,11 @@ fn parse_handler_configuration_script(
         &mut stack,
         &block,
         PipelineData::empty(),
-    )
-    .map_err(|err| {
-        let working_set = nu_protocol::engine::StateWorkingSet::new(&engine.state);
-        Error::from(nu_protocol::format_shell_error(&working_set, &err))
-    })?;
+    )?;
 
     let config = result.into_value(nu_protocol::Span::unknown())?;
 
-    let run = config
-        .get_data_by_key("run")
-        .ok_or("No 'run' field found in handler configuration")?
-        .into_closure()?;
-
+    // Parse resume_from (specific to handlers)
     let resume_from = match config.get_data_by_key("resume_from") {
         Some(val) => {
             let resume_str = val.as_str().map_err(|_| "resume_from must be a string")?;
@@ -494,59 +443,21 @@ fn parse_handler_configuration_script(
         None => ResumeFrom::default(),
     };
 
-    let modules = match config.get_data_by_key("modules") {
-        Some(val) => {
-            let record = val.as_record().map_err(|_| "modules must be a record")?;
-            record
-                .iter()
-                .map(|(name, content)| {
-                    let content = content
-                        .as_str()
-                        .map_err(|_| format!("module '{}' content must be a string", name))?;
-                    Ok((name.to_string(), content.to_string()))
-                })
-                .collect::<Result<HashMap<_, _>, Error>>()?
-        }
-        None => HashMap::new(),
-    };
-
+    // Parse pulse (specific to handlers)
     let pulse = config
         .get_data_by_key("pulse")
         .map(|v| v.as_int().map_err(|_| "pulse must be an integer"))
         .transpose()?
         .map(|n| n as u64);
 
-    let return_options = if let Some(return_config) = config.get_data_by_key("return_options") {
-        let record = return_config
-            .as_record()
-            .map_err(|_| "return must be a record")?;
-
-        let suffix = record
-            .get("suffix")
-            .map(|v| v.as_str().map_err(|_| "suffix must be a string"))
-            .transpose()?
-            .map(String::from);
-
-        let ttl = record
-            .get("ttl")
-            .map(|v| serde_json::from_str(&value_to_json(v).to_string()))
-            .transpose()
-            .map_err(|e| format!("invalid TTL: {}", e))?;
-
-        Some(ReturnOptions { suffix, ttl })
-    } else {
-        None
-    };
-
     engine.state.merge_env(&mut stack)?;
 
     Ok((
-        run,
+        common_options.run,
         HandlerConfig {
             resume_from,
-            modules,
             pulse,
-            return_options,
+            return_options: common_options.return_options,
         },
     ))
 }
diff --git a/src/handlers/tests.rs b/src/handlers/tests.rs
@@ -186,7 +186,7 @@ async fn test_register_parse_error() {
         recver.recv().await.unwrap(), {
         topic: "invalid.unregistered",
         handler: frame_handler,
-        error: "MissingPositional",
+        error: "Parse error", // Expecting parse error details
     });
 
     assert_no_more_frames(&mut recver).await;
diff --git a/src/nu/config.rs b/src/nu/config.rs
diff --git a/src/nu/mod.rs b/src/nu/mod.rs

-Original file line number
+Diff line change
 store
 .aider*
 .env
 +solid-ui