cli(refactor[add/add_from_fs]): Complete refactoring to match vcspull patterns

why: Ensure consistency with project patterns and improve code maintainability.
what:
- Updated module and function docstrings to match NumPy style
- Cleaned up imports and removed unnecessary comments
- Fixed error handling to match sync.py patterns
- Fixed type annotations for mypy compliance
- Fixed non-recursive scanning bug in add_from_fs.py
- Removed unused helper functions in tests

Author: tony

src/vcspull/cli/__init__.py

@@ -50,7 +50,7 @@ def create_parser(
     get_all_subparsers: t.Literal[True],
 ) -> tuple[
     argparse.ArgumentParser,
-    argparse._SubParsersAction,
+    argparse._SubParsersAction[argparse.ArgumentParser],
     dict[str, argparse.ArgumentParser],
 ]: ...
 
@@ -63,7 +63,7 @@ def create_parser(
     | tuple[argparse.ArgumentParser, t.Any]
     | tuple[
         argparse.ArgumentParser,
-        argparse._SubParsersAction,
+        argparse._SubParsersAction[argparse.ArgumentParser],
         dict[str, argparse.ArgumentParser],
     ]
 ):
@@ -140,26 +140,18 @@ def cli(_args: list[str] | None = None) -> None:
         return
     if args.subparser_name == "sync":
         sync_parser = all_parsers["sync"]
-        sync_kwargs = {
-            "repo_patterns": args.repo_patterns
-            if hasattr(args, "repo_patterns")
-            else None,
-            "config": (
+        sync(
+            repo_patterns=args.repo_patterns if hasattr(args, "repo_patterns") else [],
+            config=(
                 pathlib.Path(args.config)
                 if hasattr(args, "config") and args.config
                 else None
             ),
-            "exit_on_error": args.exit_on_error
+            exit_on_error=args.exit_on_error
             if hasattr(args, "exit_on_error")
             else False,
-            "parser": sync_parser,
-        }
-        sync_kwargs = {
-            k: v
-            for k, v in sync_kwargs.items()
-            if v is not None or k in {"parser", "exit_on_error", "config"}
-        }
-        sync(**sync_kwargs)
+            parser=sync_parser,
+        )
     elif args.subparser_name == "add":
         all_parsers["add"]
         add_repo_kwargs = {

src/vcspull/cli/add.py

@@ -1,4 +1,4 @@
-"""Simplified CLI functionality for vcspull add."""
+"""Add repository functionality for vcspull."""
 
 from __future__ import annotations
 
@@ -8,10 +8,7 @@
 
 import yaml
 
-from vcspull.config import (
-    find_home_config_files,
-    save_config_yaml,
-)
+from vcspull.config import find_home_config_files, save_config_yaml
 
 if t.TYPE_CHECKING:
     import argparse
@@ -20,7 +17,7 @@
 
 
 def create_add_subparser(parser: argparse.ArgumentParser) -> None:
-    """Configure :py:class:`argparse.ArgumentParser` for ``vcspull add``."""
+    """Create ``vcspull add`` argument subparser."""
     parser.add_argument(
         "-c",
         "--config",
@@ -58,7 +55,7 @@ def add_repo(
     base_dir: str | None,
 ) -> None:
     """Add a repository to the vcspull configuration.
-    
+
     Parameters
     ----------
     name : str
@@ -105,7 +102,11 @@ def add_repo(
                 )
                 return
         except Exception:
-            log.exception(f"Error loading YAML from {config_file_path}. Aborting.")
+            log.error(f"Error loading YAML from {config_file_path}. Aborting.")
+            if log.isEnabledFor(logging.DEBUG):
+                import traceback
+
+                traceback.print_exc()
             return
     else:
         log.info(
@@ -134,8 +135,7 @@ def add_repo(
         raw_config[base_dir_key] = {}
     elif not isinstance(raw_config[base_dir_key], dict):
         log.error(
-            f"Configuration section '{base_dir_key}' is not a dictionary. "
-            "Aborting.",
+            f"Configuration section '{base_dir_key}' is not a dictionary. Aborting.",
         )
         return
 
@@ -159,4 +159,9 @@ def add_repo(
             f"under '{base_dir_key}'.",
         )
     except Exception:
-        log.exception(f"Error saving config to {config_file_path}")
+        log.error(f"Error saving config to {config_file_path}")
+        if log.isEnabledFor(logging.DEBUG):
+            import traceback
+
+            traceback.print_exc()
+        raise

src/vcspull/cli/add_from_fs.py

@@ -1,30 +1,36 @@
-"""CLI functionality for vcspull add-from-fs."""
+"""Filesystem scanning functionality for vcspull."""
 
 from __future__ import annotations
 
 import logging
-import os  # For os.walk
+import os
 import pathlib
-import subprocess  # For git commands
+import subprocess
 import typing as t
 
-import yaml  # For YAML processing
+import yaml
 
-from vcspull.config import (  # Assuming these are useful
-    expand_dir,
-    find_home_config_files,
-    save_config_yaml,
-)
+from vcspull.config import expand_dir, find_home_config_files, save_config_yaml
 
 if t.TYPE_CHECKING:
     import argparse
-    # Add any necessary type imports
 
 log = logging.getLogger(__name__)
 
 
 def get_git_origin_url(repo_path: pathlib.Path) -> str | None:
-    """Get the URL of the 'origin' remote for a git repository."""
+    """Get the origin URL from a git repository.
+
+    Parameters
+    ----------
+    repo_path : pathlib.Path
+        Path to the git repository
+
+    Returns
+    -------
+    str | None
+        The origin URL if found, None otherwise
+    """
     try:
         result = subprocess.run(
             ["git", "config", "--get", "remote.origin.url"],
@@ -40,7 +46,7 @@ def get_git_origin_url(repo_path: pathlib.Path) -> str | None:
 
 
 def create_add_from_fs_subparser(parser: argparse.ArgumentParser) -> None:
-    """Configure :py:class:`argparse.ArgumentParser` for ``vcspull add-from-fs``."""
+    """Create ``vcspull add-from-fs`` argument subparser."""
     parser.add_argument(
         "-c",
         "--config",
@@ -82,7 +88,21 @@ def add_from_filesystem(
     base_dir_key_arg: str | None,
     yes: bool,
 ) -> None:
-    """Scan a directory and add found git repositories to the vcspull configuration."""
+    """Scan filesystem for git repositories and add to vcspull config.
+
+    Parameters
+    ----------
+    scan_dir_str : str
+        Directory to scan for git repositories
+    config_file_path_str : str | None
+        Path to config file, or None to use default
+    recursive : bool
+        Whether to scan subdirectories recursively
+    base_dir_key_arg : str | None
+        Base directory key to use in config (overrides automatic detection)
+    yes : bool
+        Whether to skip confirmation prompt
+    """
     scan_dir = expand_dir(pathlib.Path(scan_dir_str))
 
     config_file_path: pathlib.Path
@@ -115,8 +135,12 @@ def add_from_filesystem(
                     "Aborting.",
                 )
                 return
-        except Exception as e:
-            log.exception(f"Error loading YAML from {config_file_path}. Aborting.")
+        except Exception:
+            log.error(f"Error loading YAML from {config_file_path}. Aborting.")
+            if log.isEnabledFor(logging.DEBUG):
+                import traceback
+
+                traceback.print_exc()
             return
     else:
         log.info(
@@ -127,46 +151,72 @@ def add_from_filesystem(
         tuple[str, str, str]
     ] = []  # (repo_name, repo_url, determined_base_key)
 
-    for root, dirs, _ in os.walk(scan_dir):
-        if ".git" in dirs:
-            repo_path = pathlib.Path(root)
-            repo_name = repo_path.name
-            repo_url = get_git_origin_url(repo_path)
+    if recursive:
+        for root, dirs, _ in os.walk(scan_dir):
+            if ".git" in dirs:
+                repo_path = pathlib.Path(root)
+                repo_name = repo_path.name
+                repo_url = get_git_origin_url(repo_path)
 
-            if not repo_url:
-                log.warning(
-                    f"Could not determine remote URL for git repository at "
-                    f"{repo_path}. Skipping.",
-                )
-                continue
-
-            determined_base_key: str
-            if base_dir_key_arg:
-                determined_base_key = (
-                    base_dir_key_arg
-                    if base_dir_key_arg.endswith("/")
-                    else base_dir_key_arg + "/"
-                )
-            else:
-                # Try to make it relative to home if possible, otherwise absolute path
-                # of scan_dir parent. This should be the parent of the repo_path itself,
-                # relative to scan_dir or absolute. Or simply use scan_dir as the key.
-                # Let's use the provided scan_dir (normalized) as the default key if
-                # not given.
-                try:
+                if not repo_url:
+                    log.warning(
+                        f"Could not determine remote URL for git repository at "
+                        f"{repo_path}. Skipping.",
+                    )
+                    continue
+
+                determined_base_key: str
+                if base_dir_key_arg:
                     determined_base_key = (
-                        "~/" + str(scan_dir.relative_to(pathlib.Path.home())) + "/"
+                        base_dir_key_arg
+                        if base_dir_key_arg.endswith("/")
+                        else base_dir_key_arg + "/"
                     )
-                except ValueError:
-                    determined_base_key = str(scan_dir.resolve()) + "/"
+                else:
+                    try:
+                        determined_base_key = (
+                            "~/" + str(scan_dir.relative_to(pathlib.Path.home())) + "/"
+                        )
+                    except ValueError:
+                        determined_base_key = str(scan_dir.resolve()) + "/"
 
-            if not determined_base_key.endswith("/"):  # Ensure trailing slash
-                determined_base_key += "/"
+                if not determined_base_key.endswith("/"):
+                    determined_base_key += "/"
 
-            found_repos.append((repo_name, repo_url, determined_base_key))
+                found_repos.append((repo_name, repo_url, determined_base_key))
+    else:
+        # Non-recursive: only check immediate subdirectories
+        for item in scan_dir.iterdir():
+            if item.is_dir() and (item / ".git").is_dir():
+                repo_name = item.name
+                repo_url = get_git_origin_url(item)
+
+                if not repo_url:
+                    log.warning(
+                        f"Could not determine remote URL for git repository at "
+                        f"{item}. Skipping.",
+                    )
+                    continue
+
+                determined_base_key: str
+                if base_dir_key_arg:
+                    determined_base_key = (
+                        base_dir_key_arg
+                        if base_dir_key_arg.endswith("/")
+                        else base_dir_key_arg + "/"
+                    )
+                else:
+                    try:
+                        determined_base_key = (
+                            "~/" + str(scan_dir.relative_to(pathlib.Path.home())) + "/"
+                        )
+                    except ValueError:
+                        determined_base_key = str(scan_dir.resolve()) + "/"
 
-        if not recursive:
-            break  # Only process top-level if not recursive
+                if not determined_base_key.endswith("/"):
+                    determined_base_key += "/"
+
+                found_repos.append((repo_name, repo_url, determined_base_key))
 
     if not found_repos:
         log.info(f"No git repositories found in {scan_dir}. Nothing to add.")
@@ -223,13 +273,17 @@ def add_from_filesystem(
                 f"Adding '{repo_name}' ({repo_url}) under '{determined_base_key}'.",
             )
             changes_made = True
-        # else: already handled by the confirmation logic or --yes skip
 
     if changes_made:
         try:
             save_config_yaml(config_file_path, raw_config)
             log.info(f"Successfully updated {config_file_path}.")
-        except Exception as e:
-            log.exception(f"Error saving config to {config_file_path}")
+        except Exception:
+            log.error(f"Error saving config to {config_file_path}")
+            if log.isEnabledFor(logging.DEBUG):
+                import traceback
+
+                traceback.print_exc()
+            raise
     else:
         log.info("No changes made to the configuration.")