simpler code

Author: hexdae

crates/pcb-diode-api/src/component.rs

@@ -236,15 +236,6 @@ fn upgrade_footprint(footprint_path: &Path) -> Result<()> {
         .run()
 }
 
-fn embed_step_in_footprint(
-    footprint_content: String,
-    step_bytes: Vec<u8>,
-    step_filename: &str,
-) -> Result<String> {
-    pcb_sexpr::board::embed_step_in_footprint(footprint_content, step_bytes, step_filename)
-        .map_err(|e| anyhow::anyhow!(e))
-}
-
 /// Embed a STEP file into a footprint file, writing the result atomically.
 /// Optionally deletes the standalone STEP file after embedding.
 fn embed_step_into_footprint_file(
@@ -260,7 +251,9 @@ fn embed_step_into_footprint_file(
         .and_then(|n| n.to_str())
         .unwrap_or("model.step");
 
-    let embedded_content = embed_step_in_footprint(footprint_content, step_bytes, step_filename)?;
+    let embedded_content =
+        pcb_sexpr::board::embed_step_in_footprint(footprint_content, step_bytes, step_filename)
+            .map_err(|e| anyhow::anyhow!(e))?;
 
     // Normalize line endings and write to temporary file
     let normalized_content = embedded_content.replace("\r\n", "\n");
@@ -1565,8 +1558,12 @@ mod tests {
 	)
 )"#;
         let step_data = b"STEP DATA HERE".to_vec();
-        let result =
-            embed_step_in_footprint(footprint.to_string(), step_data, "test.step").unwrap();
+        let result = pcb_sexpr::board::embed_step_in_footprint(
+            footprint.to_string(),
+            step_data,
+            "test.step",
+        )
+        .unwrap();
 
         // Verify balanced parentheses
         assert_eq!(
@@ -1600,7 +1597,9 @@ mod tests {
 	)
 )"#;
         let step_data = b"NEW STEP DATA".to_vec();
-        let result = embed_step_in_footprint(footprint.to_string(), step_data, "new.step").unwrap();
+        let result =
+            pcb_sexpr::board::embed_step_in_footprint(footprint.to_string(), step_data, "new.step")
+                .unwrap();
 
         // Verify the transforms were preserved
         assert!(result.contains("(xyz 1 2 3)"));

crates/pcb-sexpr/src/board.rs

@@ -960,10 +960,12 @@ pub fn embed_step_in_footprint(
 #[cfg(feature = "embed")]
 pub fn extract_model_paths(footprint_text: &str) -> Vec<String> {
     use regex::Regex;
-    let model_pattern = Regex::new(r#"(?m)^\s*\(model\s+"([^"]+)""#).unwrap();
+    // Match both quoted ("path") and unquoted (path) model references,
+    // consistent with `replace_model_path`.
+    let model_pattern = Regex::new(r#"(?m)^\s*\(model\s+(?:"([^"]+)"|([^\s)]+))"#).unwrap();
     model_pattern
         .captures_iter(footprint_text)
-        .map(|caps| caps[1].to_string())
+        .filter_map(|caps| caps.get(1).or(caps.get(2)).map(|m| m.as_str().to_string()))
         .collect()
 }
 

crates/pcb/src/import/extract.rs

@@ -239,7 +239,7 @@ fn extract_kicad_schematic_data(
 
             let properties: BTreeMap<String, String> = sexpr_kicad::schematic_properties(sym)
                 .into_iter()
-                .map(|(k, v)| (k, variable_resolver.expand_best_effort(&v)))
+                .map(|(k, v)| (k, variable_resolver.expand_tolerant(&v)))
                 .collect();
             let lib_id = sexpr_kicad::string_prop(sym, "lib_id").map(KiCadLibId::from);
 
@@ -397,7 +397,7 @@ fn resolve_sheet_file(
         return None;
     }
 
-    let expanded = variable_resolver.expand_best_effort(raw);
+    let expanded = variable_resolver.expand_tolerant(raw);
 
     let candidate = PathBuf::from(&expanded);
     if candidate.is_absolute() {
@@ -550,7 +550,7 @@ fn extract_kicad_layout_data(
         let expanded_properties: BTreeMap<String, String> = fp
             .properties
             .into_iter()
-            .map(|(k, v)| (k, variable_resolver.expand_best_effort(&v)))
+            .map(|(k, v)| (k, variable_resolver.expand_tolerant(&v)))
             .collect();
 
         let layout = ImportLayoutComponent {
@@ -670,8 +670,8 @@ fn parse_kicad_sexpr_netlist_components(
             .with_context(|| format!("Netlist component {refdes} missing sheetpath (tstamps)"))?;
 
         let footprint = sexpr_kicad::string_prop(comp, "footprint");
-        let value = sexpr_kicad::string_prop(comp, "value")
-            .map(|v| variable_resolver.expand_best_effort(&v));
+        let value =
+            sexpr_kicad::string_prop(comp, "value").map(|v| variable_resolver.expand_tolerant(&v));
 
         let normalized_sheetpath_tstamps = normalize_sheetpath_tstamps(&sheetpath_tstamps);
 

crates/pcb/src/import/generate.rs

@@ -2442,6 +2442,10 @@ fn embed_models_in_footprint(
         match embed_step_in_footprint(result.clone(), step_bytes, step_filename) {
             Ok(embedded) => {
                 result = embedded;
+                // `embed_step_in_footprint` replaces ALL model paths and adds a
+                // single `(embedded_files)` block, so we can only safely embed one
+                // STEP model per footprint. Break after the first success.
+                break;
             }
             Err(e) => {
                 debug!("Failed to embed 3D model '{}': {}", step_filename, e);

crates/pcb/src/import/portable.rs

@@ -1148,19 +1148,7 @@ impl KicadVariableResolver {
         }
     }
 
-    /// Expand variables in `input`, keeping the original text for any
-    /// variable that cannot be resolved (with a debug log).
-    pub(super) fn expand_best_effort(&self, input: &str) -> String {
-        match self.expand(input) {
-            Ok(v) => v,
-            Err(e) => {
-                log::debug!("Variable expansion skipped: {}", e);
-                input.to_string()
-            }
-        }
-    }
-
-    /// Like [`Self::expand_best_effort`], but tolerant of unknown variables:
+    /// Tolerant variable expansion:
     /// known variables are replaced with their values while unknown ones are
     /// kept as literal `${NAME}` text. Useful for expanding an entire
     /// S-expression blob that may contain both project variables (e.g.
@@ -1411,35 +1399,6 @@ mod tests {
         Ok(())
     }
 
-    #[test]
-    fn expand_best_effort_resolves_known_variables() {
-        let mut vars = BTreeMap::new();
-        vars.insert("KIPRJMOD".to_string(), "/my/project".to_string());
-        vars.insert("MY_VAR".to_string(), "hello".to_string());
-        let resolver = KicadVariableResolver { vars };
-
-        assert_eq!(
-            resolver.expand_best_effort("${KIPRJMOD}/lib/foo.step"),
-            "/my/project/lib/foo.step"
-        );
-        assert_eq!(resolver.expand_best_effort("${MY_VAR}"), "hello");
-        assert_eq!(resolver.expand_best_effort("no variables"), "no variables");
-    }
-
-    #[test]
-    fn expand_best_effort_returns_original_on_unknown_variable() {
-        let resolver = KicadVariableResolver::empty();
-
-        assert_eq!(
-            resolver.expand_best_effort("${UNKNOWN_VAR}/path"),
-            "${UNKNOWN_VAR}/path"
-        );
-        assert_eq!(
-            resolver.expand_best_effort("$(ALSO_UNKNOWN)/x"),
-            "$(ALSO_UNKNOWN)/x"
-        );
-    }
-
     #[test]
     fn expand_tolerant_expands_known_and_keeps_unknown() {
         let mut vars = BTreeMap::new();