Skip to content

DBA-291 Refactor databao-cli: Phase 1#72

Closed
catstrike wants to merge 7 commits intomainfrom
ls/cli-refactoring-1
Closed

DBA-291 Refactor databao-cli: Phase 1#72
catstrike wants to merge 7 commits intomainfrom
ls/cli-refactoring-1

Conversation

@catstrike
Copy link
Collaborator

Refactor CLI command structure — move command definitions out of main.py into their own modules
__main__.py containes the full Click command definitions (decorators, options, arguments, docstrings, and handler bodies) for all CLI commands alongside their business logic. This makes __main__.py a 335-line file that mixes wiring and implementation, making it harder to locate command-specific code.

  • Each command module under commands/ should own both its *_impl function (business logic) and its @click.command() definition. Command groups should follow the same pattern via their package __init__.py
  • __main__.py should be reduced to wiring only: the root cli group, and a COMMANDS list
  • Update docs/architecture.md to document the new structure and extension points

@catstrike catstrike self-assigned this Mar 23, 2026
@catstrike catstrike requested review from a team and Copilot March 23, 2026 17:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors Databao CLI command organization by moving Click command definitions into their respective command modules and slimming __main__.py down to CLI wiring/registration, with accompanying architecture documentation updates.

Changes:

  • Moved Click decorators/handlers for commands (e.g., ask, build, index, init, status, app, mcp) into src/databao_cli/commands/*.
  • Introduced a shared CLI utility helper (get_project_or_exit) to centralize “find project” handling.
  • Updated docs/architecture.md to document the new CLI command structure and registration approach.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/databao_cli/main.py Reduced to root Click group + centralized COMMANDS registration wiring.
src/databao_cli/commands/_utils.py Adds shared helper for resolving a project (or exiting).
src/databao_cli/commands/status.py Adds Click command wrapper; switches implementation to use get_project_or_exit.
src/databao_cli/commands/init.py Adds Click command wrapper around existing init implementation and interactive flow.
src/databao_cli/commands/build.py Adds Click command wrapper for build behavior.
src/databao_cli/commands/index.py Adds Click command wrapper for indexing behavior.
src/databao_cli/commands/ask.py Adds Click command wrapper and adjusts ask_impl signature to accept a project path.
src/databao_cli/commands/app.py Adds Click command wrapper; reworks app_impl to accept explicit args/options.
src/databao_cli/commands/mcp.py Adds Click command wrapper for MCP server start.
src/databao_cli/commands/datasource/init.py Introduces datasource group and subcommands (add, check) in-package.
docs/architecture.md Documents new project/CLI structure and command registration mechanism.
Comments suppressed due to low confidence (2)

src/databao_cli/commands/status.py:34

  • status_impl() now calls get_project_or_exit(), which sys.exit(1)s when no Databao project is found. This breaks existing behavior where databao status can run outside a project (see tests) and also breaks UI usage (ui/services/dce_operations.get_status_info() imports and calls status_impl()), since exiting would terminate the Streamlit process. Consider keeping status_impl() non-exiting (use find_project() and include domain info only when available), and apply the “exit if missing” behavior only in the Click command wrapper if desired.
def status_impl(project_dir: Path) -> str:
    project_layout = get_project_or_exit(project_dir)

    dce_info = get_databao_context_engine_info()

    return _generate_info_string(
        dce_info,
        [get_databao_context_engine_domain_info(domain) for domain in project_layout.get_domain_dirs()]
        if project_layout
        else [],
    )

docs/architecture.md:66

  • This update removes the previously documented “Database Support” details (supported databases / optional deps) from the architecture doc. If that information is still accurate, consider re-adding it here or linking to the new canonical location so users don’t lose guidance on supported backends.
## Extension Points
- Add CLI command: create module in `commands/`, register with Click group
  in `__main__.py`.
- Add MCP tool: add handler in `mcp/tools/`, register in `mcp/server.py`.
- Add datasource type: extend via `databao-context-engine` optional deps.
- Add UI page: add to `ui/pages/`.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 23, 2026 17:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/databao_cli/commands/status.py:34

  • status_impl() now calls get_project_or_exit(), which exits with code 1 when run outside a Databao project. This changes the CLI behavior and breaks the existing contract verified by tests/test_status.py (it invokes databao status in an empty temp dir and expects exit_code == 0 with engine/plugin info). Consider using find_project(project_dir) here and only adding domain info when a project is found, while still returning system-wide DCE info when no project exists.
def status_impl(project_dir: Path) -> str:
    project_layout = get_project_or_exit(project_dir)

    dce_info = get_databao_context_engine_info()

    return _generate_info_string(
        dce_info,
        [get_databao_context_engine_domain_info(domain) for domain in project_layout.get_domain_dirs()]
        if project_layout
        else [],
    )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/databao_cli/commands/status.py:34

  • status_impl() now calls get_project_or_exit(), which will sys.exit(1) when run outside a Databao project. This breaks databao status in a non-project directory (see tests expecting exit_code==0) and can also terminate callers like ui/services/dce_operations.get_status_info(). Use find_project() (allowing None) in status_impl, and keep the “no project” behavior as “system-wide info only” rather than exiting; also remove the now-dead if project_layout else [] branch once project_layout is optional again.
@click.command()
@click.pass_context
def status(ctx: click.Context) -> None:
    """Display project status and system-wide information."""
    click.echo(status_impl(ctx.obj["project_dir"]))


def status_impl(project_dir: Path) -> str:
    project_layout = get_project_or_exit(project_dir)

    dce_info = get_databao_context_engine_info()

    return _generate_info_string(
        dce_info,
        [get_databao_context_engine_domain_info(domain) for domain in project_layout.get_domain_dirs()]
        if project_layout
        else [],
    )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 23, 2026 19:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/databao_cli/commands/datasource/check.py:51

  • fq_datasource_name is built using os.pathsep, which is platform-dependent (; on Windows). This output looks like a logical identifier (and tests expect root:...), so using a fixed separator (e.g., ':') would keep CLI output stable across platforms.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 23, 2026 19:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Andrei Gasparian and others added 2 commits March 24, 2026 11:55
Both test_build and test_index called duckdb.execute() on the shared
default in-memory connection instead of the file-scoped connection,
causing "table t1 already exists" when both tests ran in the same suite.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@catstrike catstrike closed this Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants