From 2418e12c6a3c234dda032301462aa2f74f1ad2f8 Mon Sep 17 00:00:00 2001 From: Tony Narlock Date: Sun, 1 Mar 2026 16:01:49 -0600 Subject: [PATCH 1/2] refactor(cli) Remove redundant traceback.print_exc after log.exception why: log.exception already includes the full traceback in logging output. what: - Remove 11 redundant traceback.print_exc() blocks from add.py, discover.py, fmt.py - Replace traceback.print_exc in sync.py with log.debug(exc_info=True) - Remove unused import traceback in add.py, discover.py, fmt.py --- src/vcspull/cli/add.py | 11 ----------- src/vcspull/cli/discover.py | 7 ------- src/vcspull/cli/fmt.py | 5 ----- src/vcspull/cli/sync.py | 5 +---- 4 files changed, 1 insertion(+), 27 deletions(-) diff --git a/src/vcspull/cli/add.py b/src/vcspull/cli/add.py index 40dfc4548..b501172ea 100644 --- a/src/vcspull/cli/add.py +++ b/src/vcspull/cli/add.py @@ -7,7 +7,6 @@ import logging import pathlib import subprocess -import traceback import typing as t from colorama import Fore, Style @@ -408,8 +407,6 @@ def add_repo( "Error loading YAML from %s. Aborting.", PrivatePath(config_file_path), ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return else: raw_config = {} @@ -589,8 +586,6 @@ def _prepare_no_merge_items( "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.", @@ -648,8 +643,6 @@ def _prepare_no_merge_items( "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) @@ -735,8 +728,6 @@ def _prepare_no_merge_items( "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"] @@ -797,5 +788,3 @@ def _prepare_no_merge_items( "Error saving config to %s", PrivatePath(config_file_path), ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() diff --git a/src/vcspull/cli/discover.py b/src/vcspull/cli/discover.py index 4902b1f9c..d376afedb 100644 --- a/src/vcspull/cli/discover.py +++ b/src/vcspull/cli/discover.py @@ -7,7 +7,6 @@ import os import pathlib import subprocess -import traceback import typing as t from colorama import Fore, Style @@ -331,8 +330,6 @@ def discover_repos( "Error loading YAML from %s. Aborting.", PrivatePath(config_file_path), ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return if raw_config is None: raw_config = {} @@ -609,8 +606,6 @@ def discover_repos( "Error saving config to %s", PrivatePath(config_file_path), ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return return @@ -710,8 +705,6 @@ def discover_repos( "Error saving config to %s", PrivatePath(config_file_path), ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return else: log.info( diff --git a/src/vcspull/cli/fmt.py b/src/vcspull/cli/fmt.py index 719f2bf46..cc801dfd7 100644 --- a/src/vcspull/cli/fmt.py +++ b/src/vcspull/cli/fmt.py @@ -6,7 +6,6 @@ import copy import logging import pathlib -import traceback import typing as t from colorama import Fore, Style @@ -188,8 +187,6 @@ def format_single_config( "Error loading config from %s", PrivatePath(config_file_path), ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return False # Format the configuration @@ -371,8 +368,6 @@ def format_single_config( "Error saving formatted config to %s", PrivatePath(config_file_path), ) - if log.isEnabledFor(logging.DEBUG): - traceback.print_exc() return False else: log.info( diff --git a/src/vcspull/cli/sync.py b/src/vcspull/cli/sync.py index fadf6e93d..808f7c492 100644 --- a/src/vcspull/cli/sync.py +++ b/src/vcspull/cli/sync.py @@ -869,10 +869,7 @@ def silent_progress(output: str, timestamp: datetime) -> None: "Failed syncing %s", repo_name, ) - if log.isEnabledFor(logging.DEBUG): - import traceback - - traceback.print_exc() + log.debug("Traceback for %s", repo_name, exc_info=True) formatter.emit_text( f"{colors.error('āœ—')} Failed syncing {colors.info(repo_name)}: " f"{colors.error(str(e))}", From d78e9a23d30123138d6f02d00b502b48e566662f Mon Sep 17 00:00:00 2001 From: Tony Narlock Date: Sun, 1 Mar 2026 16:03:30 -0600 Subject: [PATCH 2/2] refactor(test) Extract shared MockImporter to conftest why: MockImporter and CapturingMockImporter are test helpers that can be shared across CLI test files. what: - Move MockImporter and CapturingMockImporter to tests/cli/conftest.py - Update test_import_repos.py to import from conftest --- tests/cli/conftest.py | 56 ++++++++++++++++++++++++++++++++++ tests/cli/test_import_repos.py | 47 +--------------------------- 2 files changed, 57 insertions(+), 46 deletions(-) create mode 100644 tests/cli/conftest.py diff --git a/tests/cli/conftest.py b/tests/cli/conftest.py new file mode 100644 index 000000000..c8f74a2b1 --- /dev/null +++ b/tests/cli/conftest.py @@ -0,0 +1,56 @@ +"""Shared test helpers for vcspull CLI tests.""" + +from __future__ import annotations + +import typing as t + +from vcspull._internal.remotes import ( + ImportOptions, + RemoteRepo, +) + + +class MockImporter: + """Reusable mock importer for tests.""" + + def __init__( + self, + *, + service_name: str = "MockService", + repos: list[RemoteRepo] | None = None, + error: Exception | None = None, + ) -> None: + self.service_name = service_name + self._repos = repos or [] + self._error = error + + def fetch_repos( + self, + options: ImportOptions, + ) -> t.Iterator[RemoteRepo]: + """Yield mock repos or raise a mock error.""" + if self._error: + raise self._error + yield from self._repos + + +class CapturingMockImporter: + """Mock importer that captures the ImportOptions passed to fetch_repos.""" + + def __init__( + self, + *, + service_name: str = "MockService", + repos: list[RemoteRepo] | None = None, + ) -> None: + self.service_name = service_name + self._repos = repos or [] + self.captured_options: ImportOptions | None = None + + def fetch_repos( + self, + options: ImportOptions, + ) -> t.Iterator[RemoteRepo]: + """Capture options and yield repos.""" + self.captured_options = options + yield from self._repos diff --git a/tests/cli/test_import_repos.py b/tests/cli/test_import_repos.py index 0df5ebdc0..14d09c2c7 100644 --- a/tests/cli/test_import_repos.py +++ b/tests/cli/test_import_repos.py @@ -10,6 +10,7 @@ import pytest +from tests.cli.conftest import CapturingMockImporter, MockImporter from vcspull._internal.remotes import ( AuthenticationError, ConfigurationError, @@ -55,52 +56,6 @@ def _make_repo( ) -class MockImporter: - """Reusable mock importer for tests.""" - - def __init__( - self, - *, - service_name: str = "MockService", - repos: list[RemoteRepo] | None = None, - error: Exception | None = None, - ) -> None: - self.service_name = service_name - self._repos = repos or [] - self._error = error - - def fetch_repos( - self, - options: ImportOptions, - ) -> t.Iterator[RemoteRepo]: - """Yield mock repos or raise a mock error.""" - if self._error: - raise self._error - yield from self._repos - - -class CapturingMockImporter: - """Mock importer that captures the ImportOptions passed to fetch_repos.""" - - def __init__( - self, - *, - service_name: str = "MockService", - repos: list[RemoteRepo] | None = None, - ) -> None: - self.service_name = service_name - self._repos = repos or [] - self.captured_options: ImportOptions | None = None - - def fetch_repos( - self, - options: ImportOptions, - ) -> t.Iterator[RemoteRepo]: - """Capture options and yield repos.""" - self.captured_options = options - yield from self._repos - - class ResolveConfigFixture(t.NamedTuple): """Fixture for _resolve_config_file test cases."""