Private
Public Access
TIER-2 READ conductor/code_styleguides/error_handling.md end-to-end before Phase 5: refactor(gui_2): migrate L1393 _open_patch_in_external_editor to Result[T] (Phase 5)
Extract _open_patch_in_external_editor_result helper from the external editor launch try/except in App._open_patch_in_external_editor. Legacy wrapper drains errors to app._last_request_errors per FR-BC-4 event-handler pattern. [pre-audit] L1393 INTERNAL_BROAD_CATCH [post-audit] V count: 11 -> 10 (L1393 removed)
This commit is contained in:
+68
-23
@@ -1357,29 +1357,10 @@ class App:
|
|||||||
|
|
||||||
def _open_patch_in_external_editor(self) -> None:
|
def _open_patch_in_external_editor(self) -> None:
|
||||||
self._external_editor_clicked = True
|
self._external_editor_clicked = True
|
||||||
try:
|
ext_result = _open_patch_in_external_editor_result(self)
|
||||||
from src.external_editor import get_default_launcher, create_temp_modified_file
|
if not ext_result.ok:
|
||||||
import os
|
if not hasattr(self, '_last_request_errors'): self._last_request_errors = []
|
||||||
if not self._pending_patch_files:
|
self._last_request_errors.append(("_open_patch_in_external_editor", ext_result.errors[0]))
|
||||||
self._patch_error_message = "No files to edit"
|
|
||||||
return
|
|
||||||
launcher = get_default_launcher(self.config)
|
|
||||||
editor = launcher.config.get_default()
|
|
||||||
if not editor:
|
|
||||||
self._patch_error_message = "No external editor configured"
|
|
||||||
return
|
|
||||||
original_path = self._pending_patch_files[0]
|
|
||||||
if not os.path.exists(original_path):
|
|
||||||
self._patch_error_message = f"Original file not found: {original_path}"
|
|
||||||
return
|
|
||||||
temp_path = create_temp_modified_file(self._pending_patch_text)
|
|
||||||
result = launcher.launch_diff(None, original_path, temp_path)
|
|
||||||
if result is None: self._patch_error_message = "Failed to launch external editor"
|
|
||||||
else:
|
|
||||||
self._patch_error_message = None
|
|
||||||
self._vscode_diff_process = result
|
|
||||||
except Exception as e:
|
|
||||||
self._patch_error_message = str(e)
|
|
||||||
|
|
||||||
def _reorder_ticket(self, src_idx: int, dst_idx: int) -> None:
|
def _reorder_ticket(self, src_idx: int, dst_idx: int) -> None:
|
||||||
if src_idx == dst_idx: return
|
if src_idx == dst_idx: return
|
||||||
@@ -7818,6 +7799,70 @@ def _apply_pending_patch_result(app: "App") -> Result[bool]:
|
|||||||
original=e,
|
original=e,
|
||||||
)])
|
)])
|
||||||
|
|
||||||
|
|
||||||
|
def _open_patch_in_external_editor_result(app: "App") -> Result[bool]:
|
||||||
|
"""Drain-aware variant of L1393 _open_patch_in_external_editor.
|
||||||
|
|
||||||
|
Extracts the external editor launch try/except from
|
||||||
|
App._open_patch_in_external_editor into a Result-returning helper. On
|
||||||
|
success (launcher.launch_diff returns a process), sets
|
||||||
|
app._patch_error_message=None, app._vscode_diff_process=result and returns
|
||||||
|
Result(data=True). On failure (launch returns None or raises), sets
|
||||||
|
app._patch_error_message and returns Result(data=False, errors=[ErrorInfo]).
|
||||||
|
|
||||||
|
The legacy wrapper drains errors to app._last_request_errors (per FR-BC-4
|
||||||
|
event-handler drain pattern; data plane attribute).
|
||||||
|
|
||||||
|
[C: src/gui_2.py:App._open_patch_in_external_editor (L1393 legacy wrapper)]
|
||||||
|
"""
|
||||||
|
from src.external_editor import get_default_launcher, create_temp_modified_file
|
||||||
|
import os
|
||||||
|
try:
|
||||||
|
if not app._pending_patch_files:
|
||||||
|
app._patch_error_message = "No files to edit"
|
||||||
|
return Result(data=False, errors=[ErrorInfo(
|
||||||
|
kind=ErrorKind.INTERNAL,
|
||||||
|
message="No files to edit",
|
||||||
|
source="gui_2._open_patch_in_external_editor_result",
|
||||||
|
)])
|
||||||
|
launcher = get_default_launcher(app.config)
|
||||||
|
editor = launcher.config.get_default()
|
||||||
|
if not editor:
|
||||||
|
app._patch_error_message = "No external editor configured"
|
||||||
|
return Result(data=False, errors=[ErrorInfo(
|
||||||
|
kind=ErrorKind.INTERNAL,
|
||||||
|
message="No external editor configured",
|
||||||
|
source="gui_2._open_patch_in_external_editor_result",
|
||||||
|
)])
|
||||||
|
original_path = app._pending_patch_files[0]
|
||||||
|
if not os.path.exists(original_path):
|
||||||
|
app._patch_error_message = f"Original file not found: {original_path}"
|
||||||
|
return Result(data=False, errors=[ErrorInfo(
|
||||||
|
kind=ErrorKind.INTERNAL,
|
||||||
|
message=f"Original file not found: {original_path}",
|
||||||
|
source="gui_2._open_patch_in_external_editor_result",
|
||||||
|
)])
|
||||||
|
temp_path = create_temp_modified_file(app._pending_patch_text)
|
||||||
|
result = launcher.launch_diff(None, original_path, temp_path)
|
||||||
|
if result is None:
|
||||||
|
app._patch_error_message = "Failed to launch external editor"
|
||||||
|
return Result(data=False, errors=[ErrorInfo(
|
||||||
|
kind=ErrorKind.INTERNAL,
|
||||||
|
message="Failed to launch external editor",
|
||||||
|
source="gui_2._open_patch_in_external_editor_result",
|
||||||
|
)])
|
||||||
|
app._patch_error_message = None
|
||||||
|
app._vscode_diff_process = result
|
||||||
|
return Result(data=True)
|
||||||
|
except Exception as e:
|
||||||
|
app._patch_error_message = str(e)
|
||||||
|
return Result(data=False, errors=[ErrorInfo(
|
||||||
|
kind=ErrorKind.INTERNAL,
|
||||||
|
message=f"Open patch in external editor failed: {e}",
|
||||||
|
source="gui_2._open_patch_in_external_editor_result",
|
||||||
|
original=e,
|
||||||
|
)])
|
||||||
|
|
||||||
#endregion: Phase 5 Event Handler Result Helpers
|
#endregion: Phase 5 Event Handler Result Helpers
|
||||||
|
|
||||||
#endregion: MMA
|
#endregion: MMA
|
||||||
|
|||||||
@@ -923,3 +923,55 @@ def test_phase_5_l1367_apply_pending_patch_result_failure():
|
|||||||
err = result.errors[0]
|
err = result.errors[0]
|
||||||
assert err.source == "gui_2._apply_pending_patch_result"
|
assert err.source == "gui_2._apply_pending_patch_result"
|
||||||
assert "patch blew up" in err.message
|
assert "patch blew up" in err.message
|
||||||
|
|
||||||
|
|
||||||
|
def test_phase_5_l1393_open_patch_in_external_editor_result_success():
|
||||||
|
"""
|
||||||
|
L1393 _open_patch_in_external_editor_result returns Result.ok=True on success.
|
||||||
|
|
||||||
|
The helper wraps the external editor launch try/except in
|
||||||
|
App._open_patch_in_external_editor. On success (launcher.launch_diff
|
||||||
|
returns a process), returns Result(data=True).
|
||||||
|
"""
|
||||||
|
from src import gui_2
|
||||||
|
from unittest.mock import MagicMock, patch
|
||||||
|
app = MagicMock()
|
||||||
|
app._pending_patch_files = ["/proj/foo/src/foo.py"]
|
||||||
|
app._pending_patch_text = "--- a/foo.py\n+++ b/foo.py\n"
|
||||||
|
mock_editor = MagicMock(name="mock_editor")
|
||||||
|
mock_launcher = MagicMock(name="mock_launcher")
|
||||||
|
mock_launcher.config.get_default.return_value = mock_editor
|
||||||
|
mock_process = MagicMock(name="mock_process")
|
||||||
|
mock_launcher.launch_diff.return_value = mock_process
|
||||||
|
with patch("os.path.exists", return_value=True), \
|
||||||
|
patch("src.external_editor.get_default_launcher", return_value=mock_launcher), \
|
||||||
|
patch("src.external_editor.create_temp_modified_file", return_value="/tmp/patch_temp.py"):
|
||||||
|
result = gui_2._open_patch_in_external_editor_result(app)
|
||||||
|
assert result.ok, f"Expected ok=True on success, got errors: {result.errors}"
|
||||||
|
assert result.data is True
|
||||||
|
assert app._vscode_diff_process is mock_process
|
||||||
|
|
||||||
|
|
||||||
|
def test_phase_5_l1393_open_patch_in_external_editor_result_failure():
|
||||||
|
"""
|
||||||
|
L1393 _open_patch_in_external_editor_result returns Result.ok=False with ErrorInfo on failure.
|
||||||
|
|
||||||
|
When the external editor launch raises, the helper converts the exception
|
||||||
|
to ErrorInfo and returns Result(data=False, errors=[ErrorInfo]).
|
||||||
|
"""
|
||||||
|
from src import gui_2
|
||||||
|
from unittest.mock import MagicMock, patch
|
||||||
|
app = MagicMock()
|
||||||
|
app._pending_patch_files = ["/proj/foo/src/foo.py"]
|
||||||
|
app._pending_patch_text = "--- a/foo.py\n+++ b/foo.py\n"
|
||||||
|
mock_launcher = MagicMock()
|
||||||
|
mock_launcher.config.get_default.return_value = MagicMock()
|
||||||
|
with patch("os.path.exists", return_value=True), \
|
||||||
|
patch("src.external_editor.get_default_launcher", return_value=mock_launcher), \
|
||||||
|
patch("src.external_editor.create_temp_modified_file", side_effect=RuntimeError("temp file creation blew up")):
|
||||||
|
result = gui_2._open_patch_in_external_editor_result(app)
|
||||||
|
assert not result.ok, f"Expected ok=False on failure, got data: {result.data}"
|
||||||
|
assert result.errors, "Expected at least one error on failure"
|
||||||
|
err = result.errors[0]
|
||||||
|
assert err.source == "gui_2._open_patch_in_external_editor_result"
|
||||||
|
assert "temp file creation blew up" in err.message
|
||||||
|
|||||||
Reference in New Issue
Block a user