From 02fd15e55efe0a65a19cf894121716e457b9877d Mon Sep 17 00:00:00 2001 From: Tony Narlock Date: Sun, 5 Jul 2026 09:41:24 -0500 Subject: [PATCH] refactor(cli[add]) Extract duplicated save-config dispatch helpers why: master's add flow repeats the same save-with-error-handling block at six sites (three merge-path via save_config, three no-merge-path via _save_ordered_items) and derives the "current URL" from an existing entry with two identical isinstance ladders. The AddAction SKIP_PINNED/ SKIP_EXISTING split roughly doubled these over time. what: - Add _save_config_or_log_error(save_fn, *, config_file_path) wrapping the try/save/except-log-traceback pattern; each call site keeps its own success message via the returned bool - Add _extract_current_url(existing_config) for the string/dict/other URL derivation - Route all six save sites and both URL derivations through the helpers, preserving the merge vs no-merge dispatcher (save_config vs _save_ordered_items) via a passed closure - Cover both helpers with doctests and parametrized functional tests --- src/vcspull/cli/add.py | 189 ++++++++++++++++++++++++++--------------- tests/cli/test_add.py | 92 ++++++++++++++++++++ 2 files changed, 211 insertions(+), 70 deletions(-) diff --git a/src/vcspull/cli/add.py b/src/vcspull/cli/add.py index 849b1bac..18ba2386 100644 --- a/src/vcspull/cli/add.py +++ b/src/vcspull/cli/add.py @@ -382,6 +382,99 @@ def _save_ordered_items( save_config_yaml_with_items(config_file_path, items) +def _save_config_or_log_error( + save_fn: t.Callable[[], None], + *, + config_file_path: pathlib.Path, +) -> bool: + """Execute a config-save callable with standardised error handling. + + Wraps the save call in a try/except block, logging an exception message on + failure and optionally printing the traceback when DEBUG logging is active. + The success message is left to the caller, since each save site phrases it + differently (e.g. "added" vs "label adjustments saved"). + + Parameters + ---------- + save_fn : Callable[[], None] + Zero-argument callable that performs the actual file write (e.g. a + lambda wrapping ``save_config`` or ``_save_ordered_items``). + config_file_path : pathlib.Path + Path to the config file, used only for error-log messages. + + Returns + ------- + bool + ``True`` if the save succeeded, ``False`` otherwise. + + Examples + -------- + >>> import pathlib + >>> p = pathlib.Path("/tmp/x.yaml") + >>> def _ok_save() -> None: + ... pass + >>> _save_config_or_log_error(_ok_save, config_file_path=p) + True + + >>> def _bad_save() -> None: + ... raise RuntimeError("disk full") + >>> _save_config_or_log_error(_bad_save, config_file_path=p) + False + """ + try: + save_fn() + except Exception: + log.exception( + "Error saving config to %s", + PrivatePath(config_file_path), + ) + if log.isEnabledFor(logging.DEBUG): + traceback.print_exc() + return False + return True + + +def _extract_current_url(existing_config: t.Any) -> str: + """Derive a display URL from an existing repository config entry. + + Parameters + ---------- + existing_config : Any + The value stored in the workspace section for a given repo name. + May be a plain URL string, a dict with ``repo`` / ``url`` keys, + or any other object. + + Returns + ------- + str + A human-readable URL string suitable for log messages. + + Examples + -------- + >>> _extract_current_url("git+https://github.com/user/repo.git") + 'git+https://github.com/user/repo.git' + + >>> _extract_current_url({"repo": "git+https://github.com/user/repo.git"}) + 'git+https://github.com/user/repo.git' + + >>> _extract_current_url({"url": "https://example.com/repo.git"}) + 'https://example.com/repo.git' + + >>> _extract_current_url({"repo": None, "url": None}) + 'unknown' + + >>> _extract_current_url(42) + '42' + """ + if isinstance(existing_config, str): + return existing_config + if isinstance(existing_config, dict): + repo_value = existing_config.get("repo") + url_value = existing_config.get("url") + return repo_value or url_value or "unknown" + return str(existing_config) + + def handle_add_command(args: argparse.Namespace) -> None: """Entry point for the ``vcspull add`` CLI command.""" repo_input = getattr(args, "repo_path", None) @@ -790,8 +883,10 @@ def _prepare_no_merge_items( f" ({reason})" if reason else "", ) if (duplicate_merge_changes > 0 or config_was_relabelled) and not dry_run: - try: - save_config(config_file_path, raw_config) + if _save_config_or_log_error( + lambda: save_config(config_file_path, raw_config), + config_file_path=config_file_path, + ): log.info( "%s✓%s Workspace label adjustments saved to %s%s%s.", Fore.GREEN, @@ -800,13 +895,6 @@ def _prepare_no_merge_items( display_config_path, Style.RESET_ALL, ) - except Exception: - log.exception( - "Error saving config to %s", - PrivatePath(config_file_path), - ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() elif (duplicate_merge_changes > 0 or config_was_relabelled) and dry_run: log.info( "%s→%s Would save workspace label adjustments to %s%s%s.", @@ -818,14 +906,7 @@ def _prepare_no_merge_items( ) return elif add_action == AddAction.SKIP_EXISTING: - if isinstance(existing_config, str): - current_url = existing_config - elif isinstance(existing_config, dict): - repo_value = existing_config.get("repo") - url_value = existing_config.get("url") - current_url = repo_value or url_value or "unknown" - else: - current_url = str(existing_config) + current_url = _extract_current_url(existing_config) log.warning( "Repository '%s' already exists under '%s'. Current URL: %s. " @@ -836,8 +917,10 @@ def _prepare_no_merge_items( ) if (duplicate_merge_changes > 0 or config_was_relabelled) and not dry_run: - try: - save_config(config_file_path, raw_config) + if _save_config_or_log_error( + lambda: save_config(config_file_path, raw_config), + config_file_path=config_file_path, + ): log.info( "%s✓%s Workspace label adjustments saved to %s%s%s.", Fore.GREEN, @@ -846,13 +929,6 @@ def _prepare_no_merge_items( display_config_path, Style.RESET_ALL, ) - except Exception: - log.exception( - "Error saving config to %s", - PrivatePath(config_file_path), - ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() elif (duplicate_merge_changes > 0 or config_was_relabelled) and dry_run: log.info( "%s→%s Would save workspace label adjustments to %s%s%s.", @@ -886,8 +962,10 @@ def _prepare_no_merge_items( ) return - try: - save_config(config_file_path, raw_config) + if _save_config_or_log_error( + lambda: save_config(config_file_path, raw_config), + config_file_path=config_file_path, + ): log.info( "%s✓%s Successfully added %s'%s'%s (%s%s%s) to %s%s%s under '%s%s%s'.", Fore.GREEN, @@ -905,13 +983,6 @@ def _prepare_no_merge_items( workspace_label, Style.RESET_ALL, ) - except Exception: - log.exception( - "Error saving config to %s", - PrivatePath(config_file_path), - ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return ordered_items = _build_ordered_items(top_level_items, raw_config) @@ -970,8 +1041,10 @@ def _prepare_no_merge_items( Style.RESET_ALL, ) else: - try: - _save_ordered_items(config_file_path, ordered_items) + if _save_config_or_log_error( + lambda: _save_ordered_items(config_file_path, ordered_items), + config_file_path=config_file_path, + ): log.info( "%s✓%s Workspace label adjustments saved to %s%s%s.", Fore.GREEN, @@ -980,23 +1053,9 @@ def _prepare_no_merge_items( display_config_path, Style.RESET_ALL, ) - except Exception: - log.exception( - "Error saving config to %s", - PrivatePath(config_file_path), - ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return elif no_merge_add_action == AddAction.SKIP_EXISTING: - if isinstance(existing_config, str): - current_url = existing_config - elif isinstance(existing_config, dict): - repo_value = existing_config.get("repo") - url_value = existing_config.get("url") - current_url = repo_value or url_value or "unknown" - else: - current_url = str(existing_config) + current_url = _extract_current_url(existing_config) log.warning( "Repository '%s' already exists under '%s'. Current URL: %s. " @@ -1017,8 +1076,10 @@ def _prepare_no_merge_items( Style.RESET_ALL, ) else: - try: - _save_ordered_items(config_file_path, ordered_items) + if _save_config_or_log_error( + lambda: _save_ordered_items(config_file_path, ordered_items), + config_file_path=config_file_path, + ): log.info( "%s✓%s Workspace label adjustments saved to %s%s%s.", Fore.GREEN, @@ -1027,13 +1088,6 @@ def _prepare_no_merge_items( display_config_path, Style.RESET_ALL, ) - except Exception: - log.exception( - "Error saving config to %s", - PrivatePath(config_file_path), - ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return target_section = ordered_items[target_index]["section"] @@ -1067,8 +1121,10 @@ def _prepare_no_merge_items( ) return - try: - _save_ordered_items(config_file_path, ordered_items) + if _save_config_or_log_error( + lambda: _save_ordered_items(config_file_path, ordered_items), + config_file_path=config_file_path, + ): log.info( "%s✓%s Successfully added %s'%s'%s (%s%s%s) to %s%s%s under '%s%s%s'.", Fore.GREEN, @@ -1086,10 +1142,3 @@ def _prepare_no_merge_items( workspace_label, Style.RESET_ALL, ) - except Exception: - log.exception( - "Error saving config to %s", - PrivatePath(config_file_path), - ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() diff --git a/tests/cli/test_add.py b/tests/cli/test_add.py index 0e844f19..fbd9ced4 100644 --- a/tests/cli/test_add.py +++ b/tests/cli/test_add.py @@ -20,6 +20,8 @@ AddAction, _classify_add_action, _collapse_ordered_items_to_dict, + _extract_current_url, + _save_config_or_log_error, add_repo, create_add_subparser, handle_add_command, @@ -1714,3 +1716,93 @@ def test_collapse_ordered_items_to_dict( for label, expected_keys in expected_repo_keys.items(): assert label in result assert set(result[label].keys()) == expected_keys + + +class ExtractCurrentUrlFixture(t.NamedTuple): + """Fixture for _extract_current_url derivation cases.""" + + test_id: str + existing_config: object + expected: str + + +EXTRACT_CURRENT_URL_FIXTURES: list[ExtractCurrentUrlFixture] = [ + ExtractCurrentUrlFixture( + test_id="plain-string", + existing_config="git+https://github.com/user/repo.git", + expected="git+https://github.com/user/repo.git", + ), + ExtractCurrentUrlFixture( + test_id="dict-repo-key", + existing_config={"repo": "git+https://github.com/user/repo.git"}, + expected="git+https://github.com/user/repo.git", + ), + ExtractCurrentUrlFixture( + test_id="dict-url-key", + existing_config={"url": "https://example.com/repo.git"}, + expected="https://example.com/repo.git", + ), + ExtractCurrentUrlFixture( + test_id="dict-repo-wins-over-url", + existing_config={"repo": "git+ssh://a", "url": "https://b"}, + expected="git+ssh://a", + ), + ExtractCurrentUrlFixture( + test_id="dict-empty-values", + existing_config={"repo": None, "url": None}, + expected="unknown", + ), + ExtractCurrentUrlFixture( + test_id="non-str-non-dict", + existing_config=42, + expected="42", + ), +] + + +@pytest.mark.parametrize( + list(ExtractCurrentUrlFixture._fields), + EXTRACT_CURRENT_URL_FIXTURES, + ids=[fixture.test_id for fixture in EXTRACT_CURRENT_URL_FIXTURES], +) +def test_extract_current_url( + test_id: str, + existing_config: object, + expected: str, +) -> None: + """_extract_current_url derives a display URL from any entry shape.""" + assert _extract_current_url(existing_config) == expected + + +def test_save_config_or_log_error_success(tmp_path: pathlib.Path) -> None: + """A successful save callable returns True and logs no error.""" + calls: list[int] = [] + + assert ( + _save_config_or_log_error( + lambda: calls.append(1), + config_file_path=tmp_path / "cfg.yaml", + ) + is True + ) + assert calls == [1] + + +def test_save_config_or_log_error_failure( + tmp_path: pathlib.Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """A raising save callable returns False and logs the failure.""" + + def _boom() -> None: + error_message = "disk full" + raise RuntimeError(error_message) + + with caplog.at_level(logging.ERROR, logger="vcspull.cli.add"): + result = _save_config_or_log_error( + _boom, + config_file_path=tmp_path / "cfg.yaml", + ) + + assert result is False + assert any(record.levelno == logging.ERROR for record in caplog.records)