Skip to content

Commit 9628c2c

Browse files
refactor: Use config_manager.remove_value() in unset_value command (#63)
The unset_value() CLI command was directly manipulating the config file instead of using the ConfigManager.remove_value() method like other similar commands (set_value, add). This violated encapsulation and missed proper state management (cache invalidation). Also fixed ConfigManager.remove_value() to read from the specified config_path parameter instead of the default path. Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 8c0f8ca)
1 parent 4cefa73 commit 9628c2c

File tree

2 files changed

+32
-9
lines changed

2 files changed

+32
-9
lines changed

progress.md

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -780,3 +780,30 @@ This places the comment immediately before the code it documents, following the
780780

781781
---
782782

783+
## 2026-01-18: Use `config_manager.remove_value()` in `unset_value()` CLI command
784+
785+
**File:** `remote/config.py`
786+
787+
**Issue:** The `unset_value()` CLI command (lines 500-519) bypassed the `ConfigManager.remove_value()` method and directly manipulated the config file, while other similar CLI commands properly used the ConfigManager abstraction:
788+
789+
- `set_value()` correctly used `config_manager.set_value(key, value, config_path)` (line 472)
790+
- `add()` correctly used `config_manager.set_instance_name(instance_name, config_path)` (line 449)
791+
- `unset_value()` incorrectly bypassed the manager:
792+
```python
793+
config = read_config(config_path)
794+
config.remove_option("DEFAULT", key)
795+
write_config(config, config_path)
796+
```
797+
798+
This was problematic because:
799+
1. **Violated encapsulation**: The proper `ConfigManager.remove_value()` method exists but wasn't used
800+
2. **Broke consistency**: Other similar operations use the manager abstraction
801+
3. **Missing state management**: `ConfigManager.remove_value()` properly resets the cached pydantic config with `self._pydantic_config = None`, but the direct approach didn't, which could lead to stale cached configuration data
802+
803+
**Changes:**
804+
- Replaced direct config file manipulation with `config_manager.remove_value(key, config_path)`
805+
- Simplified the logic using the boolean return value to check if the key existed
806+
- Reduced code duplication by using the existing abstraction
807+
808+
---
809+

remote/config.py

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -320,17 +320,17 @@ def remove_value(self, key: str, config_path: str | None = None) -> bool:
320320
if config_path is None:
321321
config_path = str(Settings.get_config_path())
322322

323-
# Reload config to get latest state
324-
self.reload()
325-
config = self.file_config
323+
# Read from specified config path
324+
config = read_config(config_path)
326325

327326
if "DEFAULT" not in config or key not in config["DEFAULT"]:
328327
return False
329328

330329
config.remove_option("DEFAULT", key)
331330
write_config(config, config_path)
332331

333-
# Reset pydantic config to reload on next access
332+
# Reset cached configs to reload on next access
333+
self._file_config = None
334334
self._pydantic_config = None
335335
return True
336336

@@ -508,14 +508,10 @@ def unset_value(
508508
Examples:
509509
remote config unset ssh_key_path
510510
"""
511-
config = read_config(config_path)
512-
513-
if "DEFAULT" not in config or key not in config["DEFAULT"]:
511+
if not config_manager.remove_value(key, config_path):
514512
typer.secho(f"Key '{key}' not found in config", fg=typer.colors.YELLOW)
515513
raise typer.Exit(1)
516514

517-
config.remove_option("DEFAULT", key)
518-
write_config(config, config_path)
519515
typer.secho(f"Removed {key}", fg=typer.colors.GREEN)
520516

521517

0 commit comments

Comments
 (0)