-
Notifications
You must be signed in to change notification settings - Fork 3
refactor #112: reorganize config CLI and fix help-mode crash #230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Refactored the config CLI into task-oriented 'list' and 'validate' subcommands and fixed a crash that occurred when requesting help while having an invalid configuration file. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,21 @@ | ||
| """CLI interface to shows config files how docbuild sees it.""" | ||
| """CLI interface for docbuild configuration management.""" | ||
|
|
||
| import click | ||
|
|
||
| from .application import app | ||
| from .environment import env | ||
| from .list import list_config | ||
| from .validate import validate_config | ||
|
|
||
|
|
||
| @click.group( | ||
| name="config", | ||
| help=__doc__, | ||
| help="CLI interface to manage and verify configuration files.", | ||
| ) | ||
| @click.pass_context | ||
| def config(ctx: click.Context) -> None: | ||
| """Subcommand to show the configuration files and their content.""" | ||
| """Subcommand to manage docbuild configuration (TOML and XML).""" | ||
| pass | ||
|
|
||
|
|
||
| # Register the subcommands for the config group | ||
| config.add_command(env) | ||
| config.add_command(app) | ||
| # Register the task-oriented subcommands | ||
| config.add_command(list_config) | ||
| config.add_command(validate_config) |
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,62 @@ | ||||||||||||||||||||||||||||||
| """CLI interface to list the configuration.""" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import click | ||||||||||||||||||||||||||||||
| from rich import print_json | ||||||||||||||||||||||||||||||
| from rich.console import Console | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| console = Console() | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def flatten_dict(d: dict[str, Any], prefix: str = "") -> dict[str, Any]: | ||||||||||||||||||||||||||||||
| """Flatten a nested dictionary into dotted keys (e.g., app.logging.level).""" | ||||||||||||||||||||||||||||||
| items: list[tuple[str, Any]] = [] | ||||||||||||||||||||||||||||||
| for k, v in d.items(): | ||||||||||||||||||||||||||||||
| new_key = f"{prefix}.{k}" if prefix else k | ||||||||||||||||||||||||||||||
| if isinstance(v, dict): | ||||||||||||||||||||||||||||||
| items.extend(flatten_dict(v, new_key).items()) | ||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||
| items.append((new_key, v)) | ||||||||||||||||||||||||||||||
| return dict(items) | ||||||||||||||||||||||||||||||
|
Comment on lines
+11
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function looks quite general. Maybe move this function somewhere in
Comment on lines
+11
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is converting the list to a dict really necessary? It should be left to the caller. def flatten_dict(d: dict[str, Any], prefix: str = "") -> Generator[tuple[str, Any], None, None]:
"""
A generator that yields flattened key-value pairs from a nested dictionary.
:param prefix: The accumulated path of keys from higher levels of
nesting. This is used to build the dotted string (e.g., "parent.child").
Defaults to an empty string for the root level.
:yields: A tuple containing the (dotted_key, value) pair.
"""
for k, v in d.items():
# Construct the new key based on the current nesting level
new_key = f"{prefix}.{k}" if prefix else k
if isinstance(v, dict):
# yield from recursively calls the generator and yields its results
yield from flatten_dict(v, new_key)
else:
# Yield the final flattened key and its non-dict value
yield new_key, v |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _print_section(title: str, data: dict[str, Any], prefix: str, flat: bool, color: str) -> None: | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would remove the underscore here as it's not really a very secret function. I would also introduce an additional line break as it's visually more readable (I think it should pass the ruff check).
Suggested change
|
||||||||||||||||||||||||||||||
| """Print the Application and Environment configuration sections.""" | ||||||||||||||||||||||||||||||
| if flat: | ||||||||||||||||||||||||||||||
| for k, v in flatten_dict(data, prefix).items(): | ||||||||||||||||||||||||||||||
| console.print(f"[bold {color}]{k}[/bold {color}] = [green]{v}[/green]") | ||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||
| console.print(f"\n# {title}", style="blue") | ||||||||||||||||||||||||||||||
| print_json(data=data) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _print_portal(doctypes: list[Any], flat: bool) -> None: | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here:
Suggested change
|
||||||||||||||||||||||||||||||
| """Print the Portal and Doctype metadata section.""" | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you document the arguments in the docstring? |
||||||||||||||||||||||||||||||
| if not flat: | ||||||||||||||||||||||||||||||
| console.print("\n# Portal/Doctype Metadata", style="blue") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| for doctype in doctypes: | ||||||||||||||||||||||||||||||
| name = getattr(doctype, "name", "Unknown") | ||||||||||||||||||||||||||||||
| path = str(getattr(doctype, "path", "N/A")) | ||||||||||||||||||||||||||||||
| if flat: | ||||||||||||||||||||||||||||||
| console.print(f"[bold magenta]portal.{name}[/bold magenta] = [green]{path}[/green]") | ||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||
| console.print(f" - [bold]{name}[/bold]: {path}") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @click.command(name="list") | ||||||||||||||||||||||||||||||
| @click.option("--app", is_flag=True, help="Show only application configuration") | ||||||||||||||||||||||||||||||
| @click.option("--env", is_flag=True, help="Show only environment configuration") | ||||||||||||||||||||||||||||||
| @click.option("--portal", is_flag=True, help="Show only portal/doctype metadata") | ||||||||||||||||||||||||||||||
| @click.option("--flat", is_flag=True, help="Output in flat dotted format (git-style)") | ||||||||||||||||||||||||||||||
| @click.pass_context | ||||||||||||||||||||||||||||||
| def list_config(ctx: click.Context, app: bool, env: bool, portal: bool, flat: bool) -> None: | ||||||||||||||||||||||||||||||
| """List the configuration as JSON or flat text.""" | ||||||||||||||||||||||||||||||
| context = ctx.obj | ||||||||||||||||||||||||||||||
| show_all = not (app or env or portal) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (app or show_all) and context.appconfig: | ||||||||||||||||||||||||||||||
| _print_section("Application Configuration", context.appconfig.model_dump(), "app", flat, "cyan") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (env or show_all) and context.envconfig: | ||||||||||||||||||||||||||||||
| _print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow") | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why, but when I use my Traceback of docbuild config list --envIt looks like it's a problem with our models? |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (portal or show_all) and context.doctypes: | ||||||||||||||||||||||||||||||
| _print_portal(context.doctypes, flat) | ||||||||||||||||||||||||||||||
|
Comment on lines
+56
to
+62
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adjust the function names accordingly:
Suggested change
|
||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this needs to be broken down even further. Why not create a package like this: The |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,57 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """CLI interface to validate the configuration files.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import click | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from rich.console import Console | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from rich.panel import Panel | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @click.command(name="validate") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @click.pass_context | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def validate_config(ctx: click.Context) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+8
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will validate all config files. I think, that's unfortunate; maybe you only want to validate the XML? Or the env config? I would introduce the options This how you could add more arguments with an opt-in logic:
Suggested change
With that options, you would have |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Validate all configuration files (TOML and XML). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This command performs a full check of the application settings, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| environment overrides, and portal doctypes. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| context = ctx.obj | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console = Console() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Since this command is reached only if the main CLI loader | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # didn't exit with an error, we know the TOML files are | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # syntactically correct and match the Pydantic models. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print("[bold blue]Running Configuration Validation...[/bold blue]\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # 1. App Config Status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if context.appconfig: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print("✅ [bold]Application Configuration:[/bold] Valid") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if context.appconfigfiles: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for f in context.appconfigfiles: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print(f" [dim]- {f}[/dim]") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # 2. Env Config Status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if context.envconfig: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print("\n✅ [bold]Environment Configuration:[/bold] Valid") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if context.envconfigfiles: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for f in context.envconfigfiles: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print(f" [dim]- {f}[/dim]") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif context.envconfig_from_defaults: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print(" [dim]- Using internal defaults[/dim]") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # 3. Portal/Doctype Status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if context.doctypes: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print(f"\n✅ [bold]Portals/Doctypes:[/bold] {len(context.doctypes)} discovered") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for doctype in context.doctypes: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name = getattr(doctype, "name", "Unknown") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print(f" [dim]- {name}[/dim]") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+42
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not validation. Validation is much more.😉 After looking at your code, I remembered that I had done something similar for the Docserv/Portal validation. I introduced a Sorry that I didn't tell you first. I guess we should remove the |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print("\n⚠️ [bold yellow]Portals/Doctypes:[/bold yellow] None discovered") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.print( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Panel( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "[bold green]Configuration is valid![/bold green]\n" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "All TOML files match the required schema and portals are reachable.", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| border_style="green", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expand=False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| from pathlib import Path | ||
| from unittest.mock import Mock | ||
|
|
||
| import pytest | ||
|
|
||
| from docbuild.cli.cmd_cli import cli | ||
| from docbuild.models.config.app import AppConfig | ||
| from docbuild.models.config.env import EnvConfig | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_models(monkeypatch): | ||
| """Local fixture to mock config models for list command tests.""" | ||
| mock_app = Mock(spec=AppConfig) | ||
| # 1. Provide the logging attribute the CLI expects | ||
| mock_app.logging = Mock() | ||
| mock_app.logging.model_dump.return_value = {"version": 1} | ||
| # 2. Provide the data for our 'list' command | ||
| mock_app.model_dump.return_value = {"key": "value", "logging": {"level": "info"}} | ||
|
|
||
| mock_env = Mock(spec=EnvConfig) | ||
| mock_env.model_dump.return_value = {"env_key": "env_val"} | ||
|
|
||
| monkeypatch.setattr(AppConfig, "from_dict", Mock(return_value=mock_app)) | ||
| monkeypatch.setattr(EnvConfig, "from_dict", Mock(return_value=mock_env)) | ||
| return mock_app | ||
|
|
||
| def test_config_list_json(runner, mock_models, fake_handle_config): | ||
| """Test that config list shows the expected JSON output.""" | ||
| fake_handle_config(lambda *a, **k: ((Path("test.toml"),), {"key": "value"}, False)) | ||
| result = runner.invoke(cli, ["config", "list"]) | ||
| assert result.exit_code == 0 | ||
| assert '"key": "value"' in result.output | ||
|
|
||
| def test_config_list_flat(runner, mock_models, fake_handle_config): | ||
| """Test that config list with --flat shows flattened keys.""" | ||
| fake_handle_config(lambda *a, **k: ((Path("test.toml"),), {"logging": {"level": "info"}}, False)) | ||
| result = runner.invoke(cli, ["config", "list", "--flat"]) | ||
| assert result.exit_code == 0 | ||
| assert "app.logging.level = info" in result.output |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, Mock | ||
|
|
||
| import pytest | ||
|
|
||
| from docbuild.cli.cmd_cli import cli | ||
| from docbuild.cli.context import DocBuildContext | ||
| from docbuild.models.config.app import AppConfig | ||
| from docbuild.models.config.env import EnvConfig | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_models(monkeypatch): | ||
| """Local fixture to mock config models for validate command tests.""" | ||
| mock_app = Mock(spec=AppConfig) | ||
| mock_app.logging = Mock() | ||
| mock_app.logging.model_dump.return_value = {"version": 1} | ||
|
|
||
| mock_env = Mock(spec=EnvConfig) | ||
|
|
||
| monkeypatch.setattr(AppConfig, "from_dict", Mock(return_value=mock_app)) | ||
| monkeypatch.setattr(EnvConfig, "from_dict", Mock(return_value=mock_env)) | ||
| return mock_app | ||
|
|
||
| def test_config_validate_success(runner, mock_models, fake_handle_config): | ||
| """Test that config validate succeeds with valid configuration.""" | ||
| # Setup context attributes so they don't default to Mocks | ||
| # Manually create the context with real lists/tuples to avoid Mock iteration errors | ||
| ctx_obj = DocBuildContext() | ||
| ctx_obj.appconfig = mock_models | ||
| ctx_obj.appconfigfiles = (Path("app.toml"),) | ||
| ctx_obj.envconfig = MagicMock() # Mock for EnvConfig | ||
| ctx_obj.envconfigfiles = (Path("env.toml"),) | ||
| ctx_obj.doctypes = [] # Explicitly empty list | ||
|
|
||
| fake_handle_config(lambda *a, **k: ((Path("app.toml"),), {"key": "val"}, False)) | ||
|
|
||
| # We use 'standalone_mode=False' so we can see the actual error if it crashes | ||
| result = runner.invoke(cli, ["config", "validate"], obj=ctx_obj) | ||
|
|
||
| assert result.exit_code == 0 | ||
| # Use 'in' to check for text without worrying about exact formatting/colors | ||
| assert "Configuration is valid" in result.output | ||
| assert "Application Configuration" in result.output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really needed to explicitly check for
--help/-h? Shouldn't that be the task of Click? This looks like a code smell to me. 😉