refactor: Simplify add command argument parsing (Phase 2)

tony · tony · commit 68985fbcadf9 · 2025-06-21T13:40:23.000-05:00
- Replace complex repo_name_or_url parsing with explicit name and url args
- Remove 150+ lines of confusing comments and logic
- Add clear --path and --dir options for specifying location
- Make command usage intuitive: vcspull add &lt;name&gt; &lt;url&gt;
- Reduce add_repo function from 270 to 163 lines
- Improve help text and parameter documentation
diff --git a/src/vcspull/cli/__init__.py b/src/vcspull/cli/__init__.py
@@ -163,14 +163,11 @@ def cli(_args: list[str] | None = None) -> None:
     elif args.subparser_name == "add":
         all_parsers["add"]
         add_repo_kwargs = {
-            "repo_name_or_url": args.repo_name_or_url,
+            "name": args.name,
+            "url": args.url,
             "config_file_path_str": args.config if hasattr(args, "config") else None,
-            "target_path_str": args.target_path
-            if hasattr(args, "target_path")
-            else None,
-            "base_dir_key": args.base_dir_key
-            if hasattr(args, "base_dir_key")
-            else None,
+            "path": args.path if hasattr(args, "path") else None,
+            "base_dir": args.base_dir if hasattr(args, "base_dir") else None,
         }
         add_repo(**add_repo_kwargs)
     elif args.subparser_name == "add-from-fs":
diff --git a/src/vcspull/cli/add.py b/src/vcspull/cli/add.py
@@ -1,4 +1,4 @@
-"""CLI functionality for vcspull add."""
+"""Simplified CLI functionality for vcspull add."""
 
 from __future__ import annotations
 
@@ -9,7 +9,6 @@
 import yaml
 
 from vcspull.config import (
-    expand_dir,
     find_home_config_files,
     save_config_yaml,
 )
@@ -30,56 +29,70 @@ def create_add_subparser(parser: argparse.ArgumentParser) -> None:
         help="path to custom config file (default: .vcspull.yaml or ~/.vcspull.yaml)",
     )
     parser.add_argument(
-        "repo_name_or_url",
-        help="Name of the repository or its URL. "  
-        "If only name, URL may be inferred or prompted.",
+        "name",
+        help="Name for the repository in the config",
     )
     parser.add_argument(
-        "target_path",
-        nargs="?",
-        help="Optional local path for the repository (can be relative or absolute). "  
-        "Overrides default naming.",
+        "url",
+        help="Repository URL (e.g., https://github.com/user/repo.git)",
     )
     parser.add_argument(
-        "--base-dir-key",
-        help="Specify the top-level directory key from vcspull config "
-        "(e.g., '~/study/python/') under which to add this repo. "
-        "If not given, vcspull will try to infer or use the current working directory.",
+        "--path",
+        dest="path",
+        help="Local directory path where repo will be cloned "
+        "(determines base directory key if not specified with --dir)",
+    )
+    parser.add_argument(
+        "--dir",
+        dest="base_dir",
+        help="Base directory key in config (e.g., '~/projects/'). "
+        "If not specified, will be inferred from --path or use current directory.",
     )
 
 
 def add_repo(
-    repo_name_or_url: str,
+    name: str,
+    url: str,
     config_file_path_str: str | None,
-    target_path_str: str | None = None,
-    base_dir_key: str | None = None,
+    path: str | None,
+    base_dir: str | None,
 ) -> None:
-    """Add a repository to the vcspull configuration."""
+    """Add a repository to the vcspull configuration.
+    
+    Parameters
+    ----------
+    name : str
+        Repository name for the config
+    url : str
+        Repository URL
+    config_file_path_str : str | None
+        Path to config file, or None to use default
+    path : str | None
+        Local path where repo will be cloned
+    base_dir : str | None
+        Base directory key to use in config
+    """
+    # Determine config file
     config_file_path: pathlib.Path
     if config_file_path_str:
         config_file_path = pathlib.Path(config_file_path_str).expanduser().resolve()
     else:
-        # Default to ~/.vcspull.yaml if no global --config is passed
-        # Behavior of find_home_config_files might need adjustment if it returns a list
         home_configs = find_home_config_files(filetype=["yaml"])
         if not home_configs:
-            # If no global --config and no home config, default to
-            # creating .vcspull.yaml in CWD
             config_file_path = pathlib.Path.cwd() / ".vcspull.yaml"
             log.info(
                 f"No config specified and no default found, will create at "
                 f"{config_file_path}",
             )
         elif len(home_configs) > 1:
-            # This case should ideally be handled by the main CLI arg parsing
-            # for --config
             log.error(
-                "Multiple home_config files found, please specify one with -c/--config",
+                "Multiple home config files found, please specify one with -c/--config",
             )
             return
         else:
             config_file_path = home_configs[0]
 
+    # Load existing config
     raw_config: dict[str, t.Any] = {}
     if config_file_path.exists() and config_file_path.is_file():
         try:
@@ -99,163 +112,51 @@ def add_repo(
             f"Config file {config_file_path} not found. A new one will be created.",
         )
 
-    repo_name: str
-    repo_url: str
-
-    # Simplistic parsing of repo_name_or_url, assuming it's a URL if it
-    # contains '://' or 'git@'. A more robust solution would involve libvcs or regex
-    if "://" in repo_name_or_url or repo_name_or_url.startswith(
-        ("git@", "git+", "hg+", "svn+"),
-    ):
-        repo_url = repo_name_or_url
-        # Try to infer name from URL
-        repo_name = pathlib.Path(repo_url.split("/")[-1]).stem
-        if target_path_str:
-            repo_name = pathlib.Path(
-                target_path_str,
-            ).name  # if target_path is specified, use its name for repo_name
-    else:
-        repo_name = repo_name_or_url
-        # URL is not provided, for now, we can make it an error or set a
-        # placeholder. In future, could try to infer from a local git repo if
-        # target_path_str points to one
-        log.error(
-            "Repository URL must be provided if repo_name_or_url is not a URL. "
-            "Functionality to infer URL is not yet implemented.",
-        )
-        # For now, let's assume a placeholder or require URL to be part of
-        # repo_name_or_url. This part needs a decision: either make URL mandatory
-        # with name, or implement inference. Let's require the user to provide a
-        # URL for now by just using repo_name_or_url as URL if it seems like one.
-        # This means the first arg must be a URL if only one arg is given and it's
-        # not just a name. To simplify, let's assume if not detected as URL, then
-        # it's just a name and we need target_path to be a URL. Or, the user
-        # *must* provide two arguments: name then url. The current subparser takes
-        # one `repo_name_or_url` and optional `target_path`. This argument parsing
-        # needs refinement in `create_add_subparser` to accept name and url
-        # separately. For now, this will likely fail if a plain name is given
-        # without a clear URL source. Let's adjust the expectation: repo_name_or_url
-        # IS THE URL, and repo_name is derived or taken from target_path. This means
-        # first arg is always URL-like or repo name. Second is target_path (which
-        # becomes name)
-
-        # Re-evaluating argument strategy based on user request for
-        # `vcspull add my-repo <url>`: `repo_name_or_url` will be `my-repo`
-        # `target_path` will be `<url>`. This means `create_add_subparser` needs
-        # adjustment. For now, let's proceed with the current subparser and assume
-        # `repo_name_or_url` is the name, and `target_path` (if provided) is the
-        # URL for this specific case. This is getting complicated. Let's simplify
-        # the add command for now: `vcspull add <NAME> <URL>`
-        # The current `repo_name_or_url` and `target_path` can be repurposed.
-
-        # Let's rename parser args for clarity: repo_name, repo_url
-        # This requires changing create_add_subparser and the cli call in __init__.py
-        # Given the current structure, repo_name_or_url = actual name,
-        # target_path = actual_url
-        if target_path_str is None:
-            log.error("If first argument is a name, second argument (URL) is required.")
-            return
-        repo_name = repo_name_or_url
-        repo_url = target_path_str
-
-    # Determine the base directory key for the config
-    actual_base_dir_key: str
-    if base_dir_key:
-        actual_base_dir_key = (
-            base_dir_key if base_dir_key.endswith("/") else base_dir_key + "/"
-        )
-    elif target_path_str and not (
-        "://" in target_path_str or target_path_str.startswith("git@")
-    ):  # if target_path is a path, not url
-        # Infer from target_path_str if it's a local path. This assumes
-        # target_path_str is the actual clone path if provided and is not the URL
-        # itself. This logic is getting tangled due to the argument overloading.
-        # Let's stick to the idea that repo_name is derived or is first arg, url
-        # is second or inferred. For simplicity, let's assume if base_dir_key is
-        # not given, we use a default or CWD based key. This needs to be more
-        # robust. Example: if repo is to be cloned to /home/user/study/python/myrepo
-        # and vcspull.yaml has '~/study/python/': ..., then this is the key.
-        # We can use os.path.commonpath or iterate through existing keys.
-        # For now, default to current working directory's representation or a new
-        # top-level key if no match.
-        cwd_path = pathlib.Path.cwd()
-        resolved_target_dir_parent = (
-            pathlib.Path(target_path_str).parent.resolve()
-            if target_path_str
-            and not ("://" in target_path_str or target_path_str.startswith("git@"))
-            else cwd_path
-        )
-
-        found_key = None
-        for key_path_str in raw_config:
-            expanded_key_path = expand_dir(pathlib.Path(key_path_str), cwd_path)
-            if (
-                resolved_target_dir_parent == expanded_key_path
-                or resolved_target_dir_parent.is_relative_to(expanded_key_path)
-            ):
-                # Check if resolved_target_dir_parent is a subdirectory or same as
-                # an existing key path
-                try:
-                    if resolved_target_dir_parent.relative_to(expanded_key_path):
-                        found_key = key_path_str
-                        break
-                except ValueError:  # Not a sub-path
-                    if resolved_target_dir_parent == expanded_key_path:
-                        found_key = key_path_str
-                        break
-        if found_key:
-            actual_base_dir_key = found_key
-        else:
-            # Default to a key based on the target repo's parent directory or CWD
-            # Make it relative to home if possible, or absolute
-            try:
-                actual_base_dir_key = (
-                    "~/"
-                    + str(resolved_target_dir_parent.relative_to(pathlib.Path.home()))
-                    + "/"
-                )
-            except ValueError:
-                actual_base_dir_key = str(resolved_target_dir_parent) + "/"
+    # Determine base directory key
+    if base_dir:
+        # Use explicit base directory
+        base_dir_key = base_dir if base_dir.endswith("/") else base_dir + "/"
+    elif path:
+        # Infer from provided path
+        repo_path = pathlib.Path(path).expanduser().resolve()
+        try:
+            # Try to make it relative to home
+            base_dir_key = "~/" + str(repo_path.relative_to(pathlib.Path.home())) + "/"
+        except ValueError:
+            # Use absolute path
+            base_dir_key = str(repo_path) + "/"
     else:
-        # Default base directory key if not specified and target_path is not a
-        # local path
-        # This could be "." or a user-configurable default
-        actual_base_dir_key = "./"  # Default to relative current directory key
-
-    # Ensure trailing slash for consistency
-    if not actual_base_dir_key.endswith("/"):
-        actual_base_dir_key += "/"
+        # Default to current directory
+        base_dir_key = "./"
 
-    # Ensure the base directory key exists in the config
-    if actual_base_dir_key not in raw_config:
-        raw_config[actual_base_dir_key] = {}
-    elif not isinstance(raw_config[actual_base_dir_key], dict):
+    # Ensure base directory key exists in config
+    if base_dir_key not in raw_config:
+        raw_config[base_dir_key] = {}
+    elif not isinstance(raw_config[base_dir_key], dict):
         log.error(
-            f"Configuration section '{actual_base_dir_key}' is not a dictionary. "
+            f"Configuration section '{base_dir_key}' is not a dictionary. "
             "Aborting.",
         )
         return
 
-    # Check if repo already exists under this base key
-    if repo_name in raw_config[actual_base_dir_key]:
+    # Check if repo already exists
+    if name in raw_config[base_dir_key]:
         log.warning(
-            f"Repository '{repo_name}' already exists under '{actual_base_dir_key}'."
-            f" Current URL: {raw_config[actual_base_dir_key][repo_name]}."
-            f" To update, remove and re-add, or edit the YAML file manually.",
+            f"Repository '{name}' already exists under '{base_dir_key}'. "
+            f"Current URL: {raw_config[base_dir_key][name]}. "
+            f"To update, remove and re-add, or edit the YAML file manually.",
         )
         return
 
     # Add the repository
-    # Simplest form: {repo_name: repo_url}
-    # More complex form (if remotes, etc., were supported):
-    # {repo_name: {"url": repo_url, ...}}
-    raw_config[actual_base_dir_key][repo_name] = repo_url
+    raw_config[base_dir_key][name] = url
 
+    # Save config
     try:
         save_config_yaml(config_file_path, raw_config)
         log.info(
-            f"Successfully added '{repo_name}' ({repo_url}) to {config_file_path} "
-            f"under '{actual_base_dir_key}'.",
+            f"Successfully added '{name}' ({url}) to {config_file_path} "
+            f"under '{base_dir_key}'.",
         )
     except Exception:
-        log.exception(f"Error saving config to {config_file_path}")
+        log.exception(f"Error saving config to {config_file_path}")
