Remove support for "dcm cell-path" to select from record

Author: realcundo

README.md

@@ -11,11 +11,12 @@ I'm still trying to figure out what is the most useful way of using this plugin.
 send feedback in [Discussions](https://github.com/realcundo/nu_plugin_dcm/discussions) or report problems in [Issues](https://github.com/realcundo/nu_plugin_dcm/issues).
 
 ## Usage
-`dcm` plugin reads its input from single values, from specific columns, or from list of values:
-- `dcm`: expects a string/filename or binary DICOM data
-- `dcm $column_name`: reads a string/filename or binary DICOM data from `$column`. This is
-  equivalent to `get $column | dcm`.
-- `ls *.dcm | select name | dcm`: reads all files foun dby `ls` and returns a list of records.
+`dcm` plugin reads its input from single values, or from list of values:
+- `dcm`: expects a string/filename, file record (must contain `name` and `type`), dicomweb record, or binary DICOM data
+  - `ls *.dcm | dcm`: process a list of files, resulting in a list of dicom records
+  - `ls *.dcm | select name type | dcm`: process a list of files specified by their filename, resulting in a list of dicom records
+  - `open --raw file.dcm | into binary | dcm`: process a binary stream, resulting in a dicom record
+  - `open dicomweb.json | dcm`: process a dicomweb record, resulting in a dicom record
 
 See Examples for more details.
 
@@ -42,11 +43,14 @@ See Examples for more details.
 
 ### Output DICOM file as a record/table (list of records)
 ```sh
-echo file.dcm | dcm                   # uses filename/string to specify which file to open
-open --raw file.dcm | dcm             # pass binary data to `dcm`
-ls file.dcm | dcm name                # use `name` column as the filename (equivalent of `ls file.dcm | select name | dcm`)
-echo file.dcm | wrap foo | dcm foo    # use `foo` column as the filename
-open -r file.dcm | into binary | wrap foo | dcm foo # use `foo` column as binary data (see Known Limitations for details)
+echo file.dcm | dcm                      # uses filename/string to specify which file to open
+"file.dcm" | dcm                         # same asa above, uses filename/string to specify which file to open
+open --raw file.dcm | dcm                # pass (hopefully) binary data to `dcm`
+open --raw file.dcm | into binary | dcm  # pass binary data to `dcm`
+ls file.dcm | dcm                        # use file records as the filename
+ls file.dcm | select name type | dcm     # use file record-like records as the filename
+# ls file.dcm | select name | dcm        # fails because the record only contains `name` field.
+ls file.dcm | get name | dcm             # use a list of filenames (list of strings, rather than a list of records)
 ```
 
 ### Dump DICOM file as a JSON/YAML document
@@ -57,16 +61,16 @@ open -r file.dcm | dcm | to yaml
 
 ### Dump all DICOM files in the current directory to a JSON/YAML document
 ```sh
-ls *.dcm | dcm name | to json --indent 2
-ls *.dcm | dcm name | to yaml
+ls *.dcm | dcm | to json --indent 2
+ls *.dcm | dcm | to yaml
 ```
 
 ### Find all files in the current directory and subdirectories, parse them and group by Modality
 
 ```sh
 ls **/* |
   where type == file |
-  dcm name -e error |
+  dcm -e error |
   where error == "" |
   select --ignore-errors SOPInstanceUID Modality |
   group-by Modality
@@ -79,7 +83,7 @@ let files = (ls | where type == file)
 $files |
 select name size |
 merge ($files |
-  dcm name -e error |
+  dcm -e error |
   select --ignore-errors SOPInstanceUID Modality error
 ) |
 sort-by size

src/plugin.rs

@@ -10,7 +10,6 @@ use dicom::object::DefaultDicomObject;
 use dicom::object::StandardDataDictionary;
 use indexmap::IndexMap;
 use nu_plugin::{EngineInterface, Plugin, PluginCommand};
-use nu_protocol::ast::CellPath;
 use nu_protocol::{Category, Example, LabeledError, PipelineData, Record, ShellError, Signature, Span, SyntaxShape, Value};
 
 #[derive(Default)]
@@ -90,11 +89,6 @@ impl PluginCommand for DcmPluginCommand {
                 SyntaxShape::String,
                 "If an error occurs when Dicom object is parsed, the error message will be inserted in this column instead producing an error result.",
                 Some('e'))
-            .optional(
-                "column",
-                SyntaxShape::CellPath,
-                "Optional column name to use as Dicom source",
-            )
             .category(Category::Formats)  // More appropriate category
             .search_terms(vec!["dicom".to_string(), "medical".to_string(), "parse".to_string()])
             .description("Parse DICOM files and binary data")
@@ -113,9 +107,9 @@ impl PluginCommand for DcmPluginCommand {
                     ("ImageType".to_string(), Value::test_string("ORIGINAL\\PRIMARY")),
                 ]))),
             },
-            Example { description: "Parse DICOM files from a list", example: "ls *.dcm | dcm name", result: None },
+            Example { description: "Parse DICOM files from a list", example: "ls *.dcm | dcm", result: None },
             Example { description: "Parse a specific file by filename", example: "\"file.dcm\" | dcm", result: None },
-            Example { description: "Parse with error handling", example: "ls *.dcm | dcm name --error parse_error", result: None },
+            Example { description: "Parse with error handling", example: "ls *.dcm | dcm --error parse_error", result: None },
         ]
     }
 
@@ -147,14 +141,6 @@ impl PluginCommand for DcmPluginCommand {
             input.into_value(span)?
         };
 
-        // parse args, nu makes sure the num of args and type is correct
-        let source_column = call.nth(0);
-        let source_column = if let Some(Value::CellPath { val, .. }) = source_column {
-            Some(val)
-        } else {
-            None
-        };
-
         let error_column = call.get_flag_value("error");
         let error_column = if let Some(Value::String { val, .. }) = error_column {
             Some(val)
@@ -163,7 +149,7 @@ impl PluginCommand for DcmPluginCommand {
         };
 
         // run
-        let output = self.run_filter(plugin, current_dir.as_deref(), &input, source_column, error_column)?;
+        let output = self.run_filter(plugin, current_dir.as_deref(), &input, error_column)?;
 
         // Forward DataSource metadata from input to output, but clear any content type. This keeps the source.
         let output_metadata = input_metadata.map(|m| m.with_content_type(None));
@@ -178,19 +164,9 @@ impl DcmPluginCommand {
         plugin: &DcmPlugin,
         current_dir: Result<&Path, &ShellError>,
         value: &Value,
-        source_column: Option<CellPath>,
         error_column: Option<String>,
     ) -> Result<Value, LabeledError> {
-        // use source column if known
-        if let Some(source_column) = source_column {
-            let value = value.follow_cell_path(&source_column.members)?;
-
-            // AFAIK a list, process_value will handle it
-            self.process_value(plugin, current_dir, &value, &error_column)
-        } else {
-            // expect a primitive value if column is not known
-            self.process_value(plugin, current_dir, value, &error_column)
-        }
+        self.process_value(plugin, current_dir, value, &error_column)
     }
 
     fn process_value(
@@ -257,9 +233,17 @@ impl DcmPluginCommand {
 
                 if let (Some(record_type), Some(record_name)) = (record_type, record_name) {
                     if record_type == "file" {
-                        return Err(LabeledError::new("Cannot process file records directly")
-                            .with_help("Extract the filename first using: `dcm name`, `get name | dcm`, or `select name | dcm`")
-                            .with_label(format!("Found file record with name: '{record_name}'"), *internal_span));
+                        // make absolute if needed
+                        let file = resolve_path(record_name, current_dir, value.span())?;
+
+                        // merge with file-reading above (Value::String)?
+                        let obj = read_dcm_file(&file).map_err(|e| {
+                            let text = format!("{} [file {}]", e, file.to_string_lossy());
+
+                            LabeledError::new("`dcm` expects valid DICOM binary data").with_label(text, *internal_span)
+                        })?;
+
+                        return self.process_dicom_object(plugin, internal_span, obj, error_column);
                     }
                 }
 

tests/integration_tests.rs

@@ -67,7 +67,39 @@ fn test_command_vector_open() -> Result<(), nu_protocol::ShellError> {
 fn test_command_ls() -> Result<(), nu_protocol::ShellError> {
     let mut plugin_test = setup_plugin_for_test(vec![Box::new(nu_command::Ls)])?;
 
-    let result = plugin_test.eval("ls *.dcm | dcm name")?;
+    let result = plugin_test.eval("ls *.dcm | dcm")?;
+    let result = result.into_value(Span::test_data())?;
+
+    // TODO actually test the result
+    assert!(
+        result
+            .as_list()
+            .is_ok()
+    );
+    Ok(())
+}
+
+#[test]
+fn test_command_ls_select_name() -> Result<(), nu_protocol::ShellError> {
+    let mut plugin_test = setup_plugin_for_test(vec![Box::new(nu_command::Ls), Box::new(nu_command::Select)])?;
+
+    let result = plugin_test.eval("ls *.dcm | select name | dcm")?;
+    let result = result.into_value(Span::test_data())?;
+
+    // TODO actually test the result
+    assert!(
+        result
+            .as_list()
+            .is_ok()
+    );
+    Ok(())
+}
+
+#[test]
+fn test_command_ls_get_name() -> Result<(), nu_protocol::ShellError> {
+    let mut plugin_test = setup_plugin_for_test(vec![Box::new(nu_command::Ls), Box::new(nu_command::Get)])?;
+
+    let result = plugin_test.eval("ls *.dcm | get name | dcm")?;
     let result = result.into_value(Span::test_data())?;
 
     // TODO actually test the result
@@ -143,7 +175,7 @@ fn test_string_file_path_input() -> Result<(), Box<dyn std::error::Error>> {
 
     Ok(())
 }
-/// Simulate `ls *.dcm | dcm name` matching a single file
+/// Simulate `ls *.dcm | dcm` matching a single file
 #[test]
 fn test_scalar_record_input() -> Result<(), Box<dyn std::error::Error>> {
     let mut plugin_test = setup_plugin_for_test(vec![])?;
@@ -152,7 +184,7 @@ fn test_scalar_record_input() -> Result<(), Box<dyn std::error::Error>> {
     let file_record = create_file_record_value("ImplicitVRLittleEndian-Preamble.dcm");
 
     // Test with column path 'name'
-    let pipeline_data = plugin_test.eval_with("dcm name", file_record.into_pipeline_data())?;
+    let pipeline_data = plugin_test.eval_with("dcm", file_record.into_pipeline_data())?;
     let result = pipeline_data.into_value(Span::test_data())?;
 
     assert_eq!(get_string_by_cell_path(&result, "TransferSyntax"), "1.2.840.10008.1.2");
@@ -161,7 +193,7 @@ fn test_scalar_record_input() -> Result<(), Box<dyn std::error::Error>> {
     Ok(())
 }
 
-/// Simulate `ls *.dcm | dcm name` with multiple matching files
+/// Simulate `ls *.dcm | dcm` with multiple matching files
 #[test]
 fn test_vector_record_input() -> Result<(), Box<dyn std::error::Error>> {
     let mut plugin_test = setup_plugin_for_test(vec![])?;
@@ -177,7 +209,7 @@ fn test_vector_record_input() -> Result<(), Box<dyn std::error::Error>> {
     let list_value = Value::list(file_records, Span::test_data());
 
     // Test with column path 'name'
-    let pipeline_data = plugin_test.eval_with("dcm name", list_value.into_pipeline_data())?;
+    let pipeline_data = plugin_test.eval_with("dcm", list_value.into_pipeline_data())?;
     let result = pipeline_data.into_value(Span::test_data())?;
 
     assert_eq!(get_string_by_cell_path(&result, "0.PatientName"), "ExplicitVRLittleEndian-Preamble");
@@ -191,3 +223,33 @@ fn test_vector_record_input() -> Result<(), Box<dyn std::error::Error>> {
 
     Ok(())
 }
+
+/// Fail on extra parameters being passed to dcm
+#[test]
+fn test_fail_on_extra_parameters() -> Result<(), Box<dyn std::error::Error>> {
+    let mut plugin_test = setup_plugin_for_test(vec![])?;
+
+    // Create a record that simulates what 'ls' would produce
+    let file_record = create_file_record_value("ImplicitVRLittleEndian-Preamble.dcm");
+
+    // Test with column path 'name'
+    let pipeline_data = plugin_test.eval_with("dcm name", file_record.into_pipeline_data());
+
+    let error = pipeline_data.unwrap_err();
+    if let nu_protocol::ShellError::LabeledError(labeled_error) = error {
+        assert_eq!(labeled_error.msg, "Example failed to parse"); // coming from plugin test
+
+        let inner_error = labeled_error
+            .inner
+            .get(0)
+            .expect("Expected an inner error");
+
+        dbg!(&inner_error);
+        assert_eq!(inner_error.msg, "Extra positional argument.");
+        assert_eq!(inner_error.help, Some("Usage: dcm {flags} ".to_string()));
+    } else {
+        panic!("Unexpected error {:?}", error);
+    }
+
+    Ok(())
+}

tests/simple.rs

@@ -16,7 +16,7 @@ fn no_input_without_errors() {
     let p = DcmPlugin::default();
     let cmd = DcmPluginCommand;
 
-    let actual = cmd.run_filter(&p, current_dir, &Value::test_nothing(), None, None);
+    let actual = cmd.run_filter(&p, current_dir, &Value::test_nothing(), None);
 
     let expected =
         Err(LabeledError::new("Unrecognized type in stream")
@@ -34,7 +34,7 @@ fn read_explicit_vr_big_endian_preamble() {
     let p = DcmPlugin::default();
     let cmd = DcmPluginCommand;
 
-    let actual = cmd.run_filter(&p, current_dir, &filepath(filename), None, None);
+    let actual = cmd.run_filter(&p, current_dir, &filepath(filename), None);
     let actual_value = actual.unwrap();
 
     assert_eq!(get_string_by_cell_path(&actual_value, "TransferSyntax"), "1.2.840.10008.1.2.2");
@@ -52,7 +52,7 @@ fn read_explicit_vr_little_endian_preamble() {
     let p = DcmPlugin::default();
     let cmd = DcmPluginCommand;
 
-    let actual = cmd.run_filter(&p, current_dir, &filepath(filename), None, None);
+    let actual = cmd.run_filter(&p, current_dir, &filepath(filename), None);
     let actual_value = actual.unwrap();
 
     assert_eq!(get_string_by_cell_path(&actual_value, "TransferSyntax"), "1.2.840.10008.1.2.1");
@@ -69,7 +69,7 @@ fn read_implicit_vr_little_endian_preamble() {
 
     let p = DcmPlugin::default();
     let cmd = DcmPluginCommand;
-    let actual = cmd.run_filter(&p, current_dir, &filepath(filename), None, None);
+    let actual = cmd.run_filter(&p, current_dir, &filepath(filename), None);
     let actual_value = actual.unwrap();
 
     assert_eq!(get_string_by_cell_path(&actual_value, "TransferSyntax"), "1.2.840.10008.1.2");
@@ -87,7 +87,7 @@ fn read_explicit_vr_big_endian_no_preamble() {
 
     let p = DcmPlugin::default();
     let cmd = DcmPluginCommand;
-    let _actual = cmd.run_filter(&p, current_dir, &filepath(filename), None, None);
+    let _actual = cmd.run_filter(&p, current_dir, &filepath(filename), None);
 
     todo!()
 }
@@ -101,7 +101,7 @@ fn read_explicit_vr_little_endian_no_preamble() {
 
     let p = DcmPlugin::default();
     let cmd = DcmPluginCommand;
-    let _actual = cmd.run_filter(&p, current_dir, &filepath(filename), None, None);
+    let _actual = cmd.run_filter(&p, current_dir, &filepath(filename), None);
 
     todo!()
 }
@@ -115,7 +115,7 @@ fn read_implicit_vr_little_endian_no_preamble() {
 
     let p = DcmPlugin::default();
     let cmd = DcmPluginCommand;
-    let _actual = cmd.run_filter(&p, current_dir, &filepath(filename), None, None);
+    let _actual = cmd.run_filter(&p, current_dir, &filepath(filename), None);
 
     todo!()
 }