Skip to content

Commit 0a70488

Browse files
committed
fix: address twenty-ninth round of Copilot PR review feedback
- Fix TestCollectAllLayers docstring: reference collect_all_layers() - Add default/unknown strategy handling in bash/PS1 composition: error on unrecognized strategy values instead of silently skipping - Fix comment: .composed/ is a persistent dir, not temporary - Fix comment: legacy fallback checks all valid strategies, not just wrap - Cache PresetRegistry in _reconcile_skills: build presets_by_priority once instead of constructing registry per-command
1 parent a7b1660 commit 0a70488

4 files changed

Lines changed: 14 additions & 5 deletions

File tree

scripts/bash/common.sh

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,7 @@ except Exception:
577577
done
578578
content="$layer_content"
579579
;;
580+
*) echo "Error: unknown strategy '$strat'" >&2; return 1 ;;
580581
esac
581582
fi
582583
else
@@ -596,6 +597,7 @@ except Exception:
596597
done
597598
content="$layer_content"
598599
;;
600+
*) echo "Error: unknown strategy '$strat'" >&2; return 1 ;;
599601
esac
600602
fi
601603
done

scripts/powershell/common.ps1

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -547,6 +547,7 @@ except Exception:
547547
}
548548
$content = $layerContent.Replace('{CORE_TEMPLATE}', $content)
549549
}
550+
default { throw "Unknown strategy: $strat" }
550551
}
551552
}
552553
} else {
@@ -560,6 +561,7 @@ except Exception:
560561
}
561562
$content = $layerContent.Replace('{CORE_TEMPLATE}', $content)
562563
}
564+
default { throw "Unknown strategy: $strat" }
563565
}
564566
}
565567
}

src/specify_cli/presets.py

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -626,7 +626,7 @@ def _register_commands(
626626
# Resolve composed content using the full priority stack
627627
composed = resolver.resolve_content(cmd["name"], "command")
628628
if composed is not None:
629-
# Write composed content to a temporary subdirectory
629+
# Write composed content to the preset's .composed directory
630630
if composed_dir is None:
631631
composed_dir = preset_dir / ".composed"
632632
composed_dir.mkdir(parents=True, exist_ok=True)
@@ -912,6 +912,11 @@ def _reconcile_skills(self, command_names: List[str]) -> None:
912912
resolver = PresetResolver(self.project_root)
913913
skills_dir = self._get_skills_dir()
914914

915+
# Cache registry once to avoid repeated filesystem reads
916+
presets_by_priority = list(
917+
PresetRegistry(self.presets_dir).list_by_priority()
918+
) if self.presets_dir.exists() else []
919+
915920
# Group command names by winning preset to batch _register_skills calls
916921
# while only registering skills for the specific commands being reconciled.
917922
preset_cmds: Dict[str, List[str]] = {}
@@ -931,7 +936,7 @@ def _reconcile_skills(self, command_names: List[str]) -> None:
931936
if not skill_subdir.exists():
932937
# Check if any preset previously registered this skill
933938
was_managed = False
934-
for _pid, meta in PresetRegistry(self.presets_dir).list_by_priority():
939+
for _pid, meta in presets_by_priority:
935940
if not isinstance(meta, dict):
936941
continue
937942
if skill_name in meta.get("registered_skills", []):
@@ -943,7 +948,7 @@ def _reconcile_skills(self, command_names: List[str]) -> None:
943948
top_path = layers[0]["path"]
944949
# Find the preset that owns the winning layer
945950
found_preset = False
946-
for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority():
951+
for pack_id, _meta in presets_by_priority:
947952
pack_dir = self.presets_dir / pack_id
948953
if top_path.is_relative_to(pack_dir):
949954
preset_cmds.setdefault(pack_id, []).append(cmd_name)
@@ -2584,7 +2589,7 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]:
25842589
candidate = _find_in_subdirs(pack_dir)
25852590
if candidate:
25862591
# Legacy fallback: if manifest doesn't declare a strategy,
2587-
# check the command file's frontmatter for strategy: wrap
2592+
# check the command file's frontmatter for any valid strategy
25882593
if strategy == "replace" and template_type == "command":
25892594
try:
25902595
cmd_content = candidate.read_text(encoding="utf-8")

tests/test_presets.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4202,7 +4202,7 @@ def test_resolve_content_replace_over_wrap(self, project_dir, temp_dir, valid_pa
42024202

42034203

42044204
class TestCollectAllLayers:
4205-
"""Test PresetResolver._collect_all_layers() method."""
4205+
"""Test PresetResolver.collect_all_layers() method."""
42064206

42074207
def test_single_core_layer(self, project_dir):
42084208
"""Test collecting layers with only core template."""

0 commit comments

Comments
 (0)