Skip to content

Commit 3906ea6

Browse files
committed
fix: address eighth round of Copilot PR review feedback
- Hoist Get-Python3Command call out of preset loop in PowerShell Resolve-TemplateContent to avoid repeated Get-Command/version checks - Short-circuit composition when top layer is replace: in resolve_content, _reconcile_composed_commands, and bash resolve_template_content, return the top layer directly when its strategy is replace, ignoring irrelevant lower-priority composition layers - Add test: replace-over-wrap verifies that a high-priority replace preset wins without evaluating a lower-priority wrap that lacks a placeholder
1 parent 7a23eed commit 3906ea6

File tree

4 files changed

+66
-2
lines changed

4 files changed

+66
-2
lines changed

scripts/bash/common.sh

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,13 @@ except Exception:
515515
[ "$s" != "replace" ] && has_composition=true && break
516516
done
517517

518+
# If the top (highest-priority) layer is replace, it wins entirely —
519+
# lower layers are irrelevant regardless of their strategies.
520+
if [ "${layer_strategies[0]}" = "replace" ]; then
521+
cat "${layer_paths[0]}"
522+
return 0
523+
fi
524+
518525
if [ "$has_composition" = false ]; then
519526
cat "${layer_paths[0]}"
520527
return 0

scripts/powershell/common.ps1

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -413,12 +413,12 @@ function Resolve-TemplateContent {
413413
}
414414

415415
if ($sortedPresets.Count -gt 0) {
416+
$pyCmd = Get-Python3Command
416417
foreach ($presetId in $sortedPresets) {
417418
# Read strategy and file path from preset manifest
418419
$strategy = 'replace'
419420
$manifestFilePath = ''
420421
$manifest = Join-Path $presetsDir "$presetId/preset.yml"
421-
$pyCmd = Get-Python3Command
422422
if ((Test-Path $manifest) -and $pyCmd) {
423423
try {
424424
# Use Python to parse YAML manifest for strategy and file path

src/specify_cli/presets.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -628,7 +628,12 @@ def _reconcile_composed_commands(self, command_names: List[str]) -> None:
628628
if not layers:
629629
continue
630630

631-
has_composition = any(layer["strategy"] != "replace" for layer in layers)
631+
# If the top layer is replace, it wins entirely — lower layers
632+
# are irrelevant regardless of their strategies.
633+
top_is_replace = layers[0]["strategy"] == "replace"
634+
has_composition = not top_is_replace and any(
635+
layer["strategy"] != "replace" for layer in layers
636+
)
632637
if not has_composition:
633638
# Pure replace — the top layer wins.
634639
top_layer = layers[0]
@@ -2317,6 +2322,11 @@ def resolve_content(
23172322
if not layers:
23182323
return None
23192324

2325+
# If the top (highest-priority) layer is replace, it wins entirely —
2326+
# lower layers are irrelevant regardless of their strategies.
2327+
if layers[0]["strategy"] == "replace":
2328+
return layers[0]["path"].read_text(encoding="utf-8")
2329+
23202330
# Check if any layer uses a non-replace strategy
23212331
has_composition = any(layer["strategy"] != "replace" for layer in layers)
23222332

tests/test_presets.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3487,6 +3487,53 @@ def test_resolve_content_blank_line_separator(self, project_dir, temp_dir, valid
34873487
# Should have blank line separator
34883488
assert "\n\n" in content
34893489

3490+
def test_resolve_content_replace_over_wrap(self, project_dir, temp_dir, valid_pack_data):
3491+
"""Top-priority replace layer should win even if a lower layer uses wrap."""
3492+
# Install a low-priority wrap preset (with no placeholder — would fail if evaluated)
3493+
wrap_data = {**valid_pack_data}
3494+
wrap_data["preset"] = {**valid_pack_data["preset"], "id": "wrap-lo", "name": "WrapLo"}
3495+
wrap_data["provides"] = {
3496+
"templates": [{
3497+
"type": "template",
3498+
"name": "spec-template",
3499+
"file": "templates/spec-template.md",
3500+
"strategy": "wrap",
3501+
}]
3502+
}
3503+
wrap_dir = temp_dir / "wrap-lo"
3504+
wrap_dir.mkdir()
3505+
with open(wrap_dir / "preset.yml", "w") as f:
3506+
yaml.dump(wrap_data, f)
3507+
(wrap_dir / "templates").mkdir()
3508+
# Intentionally missing {CORE_TEMPLATE} — would error if composition ran
3509+
(wrap_dir / "templates" / "spec-template.md").write_text("wrapper without placeholder")
3510+
3511+
manager = PresetManager(project_dir)
3512+
manager.install_from_directory(wrap_dir, "0.1.5", priority=10)
3513+
3514+
# Install a high-priority replace preset
3515+
rep_data = {**valid_pack_data}
3516+
rep_data["preset"] = {**valid_pack_data["preset"], "id": "rep-hi", "name": "RepHi"}
3517+
rep_data["provides"] = {
3518+
"templates": [{
3519+
"type": "template",
3520+
"name": "spec-template",
3521+
"file": "templates/spec-template.md",
3522+
}]
3523+
}
3524+
rep_dir = temp_dir / "rep-hi"
3525+
rep_dir.mkdir()
3526+
with open(rep_dir / "preset.yml", "w") as f:
3527+
yaml.dump(rep_data, f)
3528+
(rep_dir / "templates").mkdir()
3529+
(rep_dir / "templates" / "spec-template.md").write_text("# Replaced content\n")
3530+
3531+
manager.install_from_directory(rep_dir, "0.1.5", priority=1)
3532+
3533+
resolver = PresetResolver(project_dir)
3534+
content = resolver.resolve_content("spec-template")
3535+
assert content == "# Replaced content\n"
3536+
34903537

34913538
class TestCollectAllLayers:
34923539
"""Test PresetResolver._collect_all_layers() method."""

0 commit comments

Comments
 (0)