Skip to content

Consolidate & fix storage docs#142

Merged
AivanF merged 1 commit into
mainfrom
aivan/2026-05-25-consolidate-Storage-docs
May 26, 2026
Merged

Consolidate & fix storage docs#142
AivanF merged 1 commit into
mainfrom
aivan/2026-05-25-consolidate-Storage-docs

Conversation

@AivanF
Copy link
Copy Markdown
Collaborator

@AivanF AivanF commented May 26, 2026

Summary by CodeRabbit

  • Documentation
    • Updated storage configuration guidance to use platform-appropriate default paths (Linux, macOS, Windows) instead of hardcoded ./slayer_data
    • Added examples for overriding storage via --storage flag and $SLAYER_STORAGE environment variable
    • Clarified storage behavior across all interfaces (CLI, MCP, REST API) documentation

Review Change Stack

@AivanF AivanF requested a review from whimo May 26, 2026 07:10
@sonarqubecloud
Copy link
Copy Markdown

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

📝 Walkthrough

Walkthrough

This PR updates SLayer storage configuration across implementation and documentation layers. The core change introduces explicit platform detection for default storage paths in slayer/storage/base.py, while documentation across guides, CLI references, and setup examples removes hardcoded ./slayer_data defaults in favor of platform-appropriate paths with clear override instructions.

Changes

Platform-aware default storage paths

Layer / File(s) Summary
Platform-aware default storage path implementation
slayer/storage/base.py
Adds sys import and updates default_storage_path() to use explicit sys.platform == "darwin" branching for macOS detection, treating other platforms as Linux/etc. Docstring clarified that macOS ignores $XDG_DATA_HOME.
Storage configuration and CLI reference documentation
docs/configuration/storage.md, docs/reference/cli.md, docs/reference/mcp.md, docs/interfaces/cli.md, CLAUDE.md
Core documentation updated to reference platform defaults instead of hardcoded paths, with new "Overriding the Storage Path" section containing CLI flag and environment-variable examples. CLI reference tables across slayer serve, slayer mcp, and slayer query link to centralized storage docs. SLAYER_INGEST_ON_STARTUP environment variable description clarified for truthy value semantics.
Getting started and integration setup examples
docs/getting-started/mcp.md, docs/interfaces/mcp.md, docs/interfaces/rest-api.md, docs/dbt/dbt_import.md
Command-line examples remove explicit --storage ./slayer_data flags and clarify platform defaults. Datasource path examples reference the configured storage path. MCP setup instructions (stdio, SSE, virtualenv) omit hardcoded storage arguments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • MotleyAI/slayer#38: Both PRs modify the storage path resolution logic in slayer/storage/base.py and update CLI documentation to reflect platform-aware default storage behavior.

Suggested reviewers

  • whimo
  • ZmeiGorynych

Poem

🐰 A rabbit hops through paths so true,
macOS here, and Linux too!
No more hardcoded ./slayer_data schemes—
Platform defaults fulfill our dreams! 🌟

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly summarizes the main changes: consolidating storage documentation across multiple files and fixing/updating storage path defaults and documentation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch aivan/2026-05-25-consolidate-Storage-docs

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/dbt/dbt_import.md (1)

10-10: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove legacy hardcoded storage path from Quick Start.

Quick Start still hardcodes --storage ./slayer_data, which conflicts with the new platform-default guidance and can mislead users into thinking the legacy path is still the default.

Suggested doc fix
- slayer import-dbt ./my_dbt_project --datasource my_postgres --storage ./slayer_data
+ slayer import-dbt ./my_dbt_project --datasource my_postgres
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/dbt/dbt_import.md` at line 10, The Quick Start example includes a legacy
hardcoded storage flag (--storage ./slayer_data) in the "slayer import-dbt
./my_dbt_project --datasource my_postgres --storage ./slayer_data" command;
remove the --storage ./slayer_data part from the example (or replace it with an
optional note such as "[--storage <path>] to override platform default") so the
example shows the platform-default behavior and avoids implying the old path is
the default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@docs/dbt/dbt_import.md`:
- Line 10: The Quick Start example includes a legacy hardcoded storage flag
(--storage ./slayer_data) in the "slayer import-dbt ./my_dbt_project
--datasource my_postgres --storage ./slayer_data" command; remove the --storage
./slayer_data part from the example (or replace it with an optional note such as
"[--storage <path>] to override platform default") so the example shows the
platform-default behavior and avoids implying the old path is the default.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 84b93603-b5b2-4d1d-9ec0-be9329a213b7

📥 Commits

Reviewing files that changed from the base of the PR and between ebd913f and 78b95f4.

📒 Files selected for processing (10)
  • CLAUDE.md
  • docs/configuration/storage.md
  • docs/dbt/dbt_import.md
  • docs/getting-started/mcp.md
  • docs/interfaces/cli.md
  • docs/interfaces/mcp.md
  • docs/interfaces/rest-api.md
  • docs/reference/cli.md
  • docs/reference/mcp.md
  • slayer/storage/base.py

@AivanF AivanF merged commit ff8d949 into main May 26, 2026
6 checks passed
Comment thread CLAUDE.md
## CLI

- All commands accept `--storage` (directory for YAML, `.db` file for SQLite). Defaults to platform-appropriate path (`~/.local/share/slayer` on Linux, `~/Library/Application Support/slayer` on macOS). Override with `$SLAYER_STORAGE` env var. Legacy `--models-dir` still works.
- All commands accept `--storage` (directory for YAML, `.db` file for SQLite). Defaults to platform-appropriate path (`~/.local/share/slayer` on Linux, `~/Library/Application Support/slayer` on macOS, `%LOCALAPPDATA%\slayer` on Windows). Override with `$SLAYER_STORAGE` env var.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be ~/.local/share/slayer on Mac as well

|---|---|
| Linux | `~/.local/share/slayer` (or `$XDG_DATA_HOME/slayer`) |
| Linux | `~/.local/share/slayer` (or `$XDG_DATA_HOME/slayer` if set) |
| macOS | `~/Library/Application Support/slayer` |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

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