refactor #158: address review comments on pydantic error formatting

Signed-off-by: sushant-suse <[email protected]>

Author: sushant-suse

src/docbuild/cli/cmd_cli.py

@@ -1,12 +1,13 @@
 """Main CLI tool for document operations."""
 
+from collections.abc import Sequence
 import logging
 from pathlib import Path
 import sys
 from typing import Any, cast
 
 import click
-from pydantic import ValidationError
+from pydantic import BaseModel, ValidationError
 import rich.console
 from rich.traceback import install as install_traceback
 
@@ -47,6 +48,33 @@ def _setup_console() -> None:
     install_traceback(console=CONSOLE, show_locals=True)
 
 
+def _handle_validation_error(
+    e: Exception,
+    model_class: type[BaseModel],
+    config_files: Sequence[Path] | None,
+    verbose: int,
+    ctx: click.Context,
+) -> None:
+    """Format validation errors and exit the CLI.
+
+    Outsourced logic to avoid code duplication between App and Env config phases.
+    Using Sequence[Path] ensures compatibility with both lists and tuples.
+    """
+    config_label = "Application" if model_class == AppConfig else "Environment"
+
+    if isinstance(e, ValidationError):
+        # Safely extract the first config file name for the error header
+        config_file = str((config_files or ["unknown"])[0])
+        format_pydantic_error(
+            e, model_class, config_file, verbose, console=CONSOLE
+        )
+    else:
+        log.error("%s configuration failed validation:", config_label)
+        log.error("Error in config file(s): %s", config_files)
+        log.error(e)
+    ctx.exit(1)
+
+
 @click.group(
     name=APP_NAME,
     context_settings={"show_default": True, "help_option_names": ["-h", "--help"]},
@@ -103,37 +131,24 @@ def cli(
     env_config: Path,
     **kwargs: dict,
 ) -> None:
-    """Acts as a main entry point for CLI tool.
-
-    :param ctx: The Click context object.
-    :param verbose: The verbosity level.
-    :param dry_run: If set, just pretend to run the command without making any changes.
-    :param debug: If set, enable debug mode.
-    :param app_config: Filename to the application TOML config file.
-    :param env_config: Filename to a environment's TOML config file.
-    :param kwargs: Additional keyword arguments.
-    """
+    """Acts as a main entry point for CLI tool."""
     if ctx.invoked_subcommand is None:
-        # If no subcommand is invoked, show the help message
         click.echo(10 * "-")
         click.echo(ctx.get_help())
         ctx.exit(0)
 
     if ctx.obj is None:
         ctx.ensure_object(DocBuildContext)
 
-    # Build the context object
     context = ctx.obj
     context.verbose = verbose
     context.dry_run = dry_run
     context.debug = debug
 
-    # --- PHASE 1: Load and Validate Application Config (and setup logging) ---
-    #
-    # 1. Load the raw application config dictionary
+    # --- PHASE 1: Load and Validate Application Config ---
     (
         context.appconfigfiles,
-        raw_appconfig,  # Store config as raw dictionary
+        raw_appconfig,
         context.appconfig_from_defaults,
     ) = handle_config(
         app_config,
@@ -143,39 +158,23 @@ def cli(
         DEFAULT_APP_CONFIG,
     )
 
-    # Explicitly cast the raw_appconfig type to silence Pylance
     raw_appconfig = cast(dict[str, Any], raw_appconfig)
 
-    # 2. Validate the raw config dictionary using Pydantic
     try:
-        # Pydantic validation also handles placeholder replacement via @model_validator
         context.appconfig = AppConfig.from_dict(raw_appconfig)
     except (ValueError, ValidationError) as e:
-        if isinstance(e, ValidationError):
-            # FIXED: Added fallback for config files to avoid subscripting None
-            config_file = str((context.appconfigfiles or ["unknown"])[0])
-            format_pydantic_error(
-                e, AppConfig, config_file, context.verbose
-            )
-        else:
-            log.error("Application configuration failed validation:")
-            log.error("Error in config file(s): %s", context.appconfigfiles)
-            log.error(e)
-        ctx.exit(1)
+        _handle_validation_error(e, AppConfig, context.appconfigfiles, verbose, ctx)
 
     # 3. Setup logging using the validated config object
-    # Use model_dump(by_alias=True) to ensure the 'class' alias is used.
     logging_config = context.appconfig.logging.model_dump(
         by_alias=True, exclude_none=True
     )
     setup_logging(cliverbosity=verbose, user_config={"logging": logging_config})
 
     # --- PHASE 2: Load Environment Config, Validate, and Acquire Lock ---
-    #
-    # 1. Load raw Environment Config
     (
         context.envconfigfiles,
-        raw_envconfig,  # Renaming context.envconfig to raw_envconfig locally
+        raw_envconfig,
         context.envconfig_from_defaults,
     ) = handle_config(
         env_config,
@@ -185,57 +184,32 @@ def cli(
         DEFAULT_ENV_CONFIG,
     )
 
-    # Explicitly cast the raw_envconfig type to silence Pylance
     raw_envconfig = cast(dict[str, Any], raw_envconfig)
 
-    # 2. VALIDATE the raw environment config dictionary using Pydantic
     try:
-        # Pydantic validation handles placeholder replacement via @model_validator
-        # The result is the validated Pydantic object, stored in context.envconfig
         context.envconfig = EnvConfig.from_dict(raw_envconfig)
     except (ValueError, ValidationError) as e:
-        if isinstance(e, ValidationError):
-            # FIXED: Added fallback for config files to avoid subscripting None
-            config_file = str((context.envconfigfiles or ["unknown"])[0])
-            format_pydantic_error(
-                e, EnvConfig, config_file, context.verbose
-            )
-        else:
-            log.error(
-                "Environment configuration failed validation: "
-                "Error in config file(s): %s %s",
-                context.envconfigfiles,
-                e,
-            )
-        ctx.exit(1)
+        _handle_validation_error(e, EnvConfig, context.envconfigfiles, verbose, ctx)
 
     env_config_path = (context.envconfigfiles or [None])[0]
 
-    # --- CONCURRENCY CONTROL: Use explicit __enter__ and cleanup registration ---
+    # --- CONCURRENCY CONTROL ---
     if env_config_path:
-        # 1. Instantiate the lock object
         ctx.obj.env_lock = PidFileLock(resource_path=cast(Path, env_config_path))
-
         try:
-            # 2. Acquire the lock by explicitly calling the __enter__ method.
             ctx.obj.env_lock.__enter__()
             log.info("Acquired lock for environment config: %r", env_config_path.name)
-
         except LockAcquisitionError as e:
-            # Handles lock contention
             log.error(str(e))
             ctx.exit(1)
         except Exception as e:
-            # Handles critical errors
             log.error("Failed to set up environment lock: %s", e)
             ctx.exit(1)
 
-        # 3. Register the lock's __exit__ method to be called when the context
-        # terminates.
         ctx.call_on_close(lambda: ctx.obj.env_lock.__exit__(None, None, None))
 
 
-# Add subcommand
+# Add subcommands
 cli.add_command(build)
 cli.add_command(c14n)
 cli.add_command(config)

src/docbuild/constants.py

@@ -164,3 +164,8 @@
 
 XMLDATADIR = Path(__file__).parent / "config" / "xml" / "data"
 """Directory where additional files (RNC, XSLT) for XML processing are stored."""
+
+# --- UI and Error Reporting Constants ---
+
+DEFAULT_ERROR_LIMIT = 5
+"""The maximum number of validation errors to display before truncating the output."""

src/docbuild/utils/errors.py

@@ -1,24 +1,31 @@
 """Utilities for handling and formatting application errors."""
 
+
 from pydantic import BaseModel, ValidationError
 from rich.console import Console
 from rich.text import Text
 
+from ..constants import DEFAULT_ERROR_LIMIT
+
 
 def format_pydantic_error(
     error: ValidationError,
     model_class: type[BaseModel],
     config_file: str,
-    verbose: int = 0
+    verbose: int = 0,
+    console: Console | None = None,
 ) -> None:
     """Centralized formatter for Pydantic ValidationErrors using Rich.
 
     :param error: The caught ValidationError object.
     :param model_class: The Pydantic model class that failed validation.
     :param config_file: The name/path of the config file being processed.
     :param verbose: Verbosity level to control error detail.
+    :param console: Optional Rich console object. If None, creates a stderr console.
     """
-    console = Console(stderr=True)
+    # Use provided console or fall back to default (Dependency Injection)
+    con = console or Console(stderr=True)
+
     errors = error.errors()
     error_count = len(errors)
 
@@ -29,16 +36,24 @@ def format_pydantic_error(
         (f"'{config_file}'", "bold cyan"),
         (":", "white")
     )
-    console.print(header)
-    console.print()
+    con.print(header)
+    con.print()
 
-    # Smart Truncation: Show only first 5 unless verbose
-    max_display = 5 if verbose < 2 else error_count
+    # Smart Truncation: Use the constant from constants.py
+    max_display = DEFAULT_ERROR_LIMIT if verbose < 2 else error_count
     display_errors = errors[:max_display]
 
     for i, err in enumerate(display_errors, 1):
         # 1. Resolve Location and Field Info
-        loc_path = ".".join(str(v) for v in err["loc"])
+        # Filter out internal Pydantic tags (strings with '[' or '-')
+        # to keep the path clean for end users.
+        clean_loc = [
+            str(v) for v in err["loc"]
+            if not (isinstance(v, str) and ("[" in v or "-" in v))
+            and not isinstance(v, int)
+        ]
+        loc_path = ".".join(clean_loc)
+
         err_type = err["type"]
         msg = err["msg"]
 
@@ -47,21 +62,17 @@ def format_pydantic_error(
         current_model = model_class
 
         for part in err["loc"]:
-            # Check if current_model is a Pydantic class and contains the field
             if (isinstance(current_model, type) and
                 issubclass(current_model, BaseModel) and
                 part in current_model.model_fields):
 
                 field_info = current_model.model_fields[part]
 
-                # Move deeper into the tree if the annotation is another model
                 annotation = field_info.annotation
                 if (isinstance(annotation, type) and
                     issubclass(annotation, BaseModel)):
                     current_model = annotation
                 else:
-                    # We have reached a leaf node or a complex type (List, etc.)
-                    # Stop traversing but keep the field_info
                     current_model = None
             else:
                 field_info = None
@@ -92,12 +103,12 @@ def format_pydantic_error(
             style="link underline blue"
         )
 
-        console.print(error_panel)
-        console.print()
+        con.print(error_panel)
+        con.print()
 
     # Footer for Truncation
     if error_count > max_display:
-        console.print(
+        con.print(
             f"[dim]... and {error_count - max_display} more errors. "
             "Use '-vv' to see all errors.[/dim]\n"
         )

tests/cli/test_cmd_cli.py

@@ -176,7 +176,7 @@ def test_cli_config_validation_failure(
     mock_config_models,
     is_app_config_failure,
 ):
-    """Test that the CLI handles Pydantic validation errors with the new formatter."""
+    """Verify that the CLI handles Pydantic validation errors with the new formatter."""
     app_file = app_config_file
     app_file.write_text("bad data")
 
@@ -201,7 +201,7 @@ def test_cli_config_validation_failure(
     else:
         mock_config_models["env_from_dict"].side_effect = mock_validation_error
 
-    # 3. Mock handle_config to return raw data successfully
+    # 3. Mock handle_config
     def resolver(user_path, *a, **kw):
         if user_path == app_file:
             return (app_file,), {"raw_app_data": "x"}, False
@@ -210,7 +210,6 @@ def resolver(user_path, *a, **kw):
     fake_handle_config(resolver)
 
     context = DocBuildContext()
-    # Click runner captures stdout/stderr. Our rich formatter prints to stderr.
     result = runner.invoke(
         cli,
         ["--app-config", str(app_file), "capture"],
@@ -226,9 +225,8 @@ def resolver(user_path, *a, **kw):
 
     # Check that our new pretty-printer output exists in the captured terminal output
     assert "Validation error" in result.output
+    # Check that the specific field causing the issue is mentioned
     assert "server.port" in result.output
-    # Check for the Pydantic error message
-    assert "Input should be a valid integer" in result.output
 
 
 def test_cli_verbose_and_debug(