Skip to content

Commit e7a0929

Browse files
committed
fix: address PR review feedback on --force implementation
- Remove unused `backup_config_dir` variable assignment (Ruff F841) - Defer `remove()` until after `_validate_install_conflicts()` to prevent data loss if validation fails mid-reinstall - Use `TemporaryDirectory` instead of `NamedTemporaryFile` in ZIP test to avoid Windows file-locking failures
1 parent 48d4e97 commit e7a0929

2 files changed

Lines changed: 11 additions & 9 deletions

File tree

src/specify_cli/extensions.py

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1164,12 +1164,7 @@ def install_from_directory(
11641164

11651165
# Check if already installed
11661166
if self.registry.is_installed(manifest.id):
1167-
if force:
1168-
# Remove existing extension first (handles command/skill/hook
1169-
# cleanup, config backup, and registry removal).
1170-
backup_config_dir = self.extensions_dir / ".backup" / manifest.id
1171-
self.remove(manifest.id)
1172-
else:
1167+
if not force:
11731168
raise ExtensionError(
11741169
f"Extension '{manifest.id}' is already installed. "
11751170
f"Use 'specify extension remove {manifest.id}' first, "
@@ -1179,6 +1174,12 @@ def install_from_directory(
11791174
# Reject manifests that would shadow core commands or installed extensions.
11801175
self._validate_install_conflicts(manifest)
11811176

1177+
# Remove existing installation AFTER all validations pass so that a
1178+
# validation failure doesn't leave the user with a half-uninstalled
1179+
# extension (configs stranded in .backup/).
1180+
if force and self.registry.is_installed(manifest.id):
1181+
self.remove(manifest.id)
1182+
11821183
# Install extension
11831184
dest_dir = self.extensions_dir / manifest.id
11841185
if dest_dir.exists():

tests/test_extensions.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -848,9 +848,10 @@ def test_install_zip_force_reinstall(self, extension_dir, project_dir):
848848
# Install once from directory
849849
manager.install_from_directory(extension_dir, "0.1.0", register_commands=False)
850850

851-
# Create a ZIP of the extension
852-
with tempfile.NamedTemporaryFile(suffix=".zip") as tmp:
853-
zip_path = Path(tmp.name)
851+
# Create a ZIP of the extension in a temp directory (not NamedTemporaryFile,
852+
# which can fail on Windows due to file locking).
853+
with tempfile.TemporaryDirectory() as tmpdir:
854+
zip_path = Path(tmpdir) / "test-ext.zip"
854855
with zipfile.ZipFile(zip_path, "w") as zf:
855856
for f in extension_dir.rglob("*"):
856857
if f.is_file():

0 commit comments

Comments
 (0)