Skip to content

Default storage path#38

Merged
AivanF merged 3 commits into
mainfrom
aivan/2026-04-17-default-models-path
Apr 17, 2026
Merged

Default storage path#38
AivanF merged 3 commits into
mainfrom
aivan/2026-04-17-default-models-path

Conversation

@AivanF
Copy link
Copy Markdown
Collaborator

@AivanF AivanF commented Apr 17, 2026

Summary by CodeRabbit

  • New Features

    • Platform-specific default storage locations (Linux, macOS, Windows) with ability to override via $SLAYER_STORAGE; CLI now displays the resolved default.
  • Documentation

    • Simplified API and MCP startup examples—no explicit storage flag required.
    • Added storage configuration guide with default-location mapping and updated MCP registration examples.
  • Roadmap

    • Marked "Smart output formatting" as complete.

@AivanF AivanF requested a review from whimo April 17, 2026 10:16
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7c899017-19b8-4225-abf1-234a9cfa2ea2

📥 Commits

Reviewing files that changed from the base of the PR and between 577663e and 2db2674.

📒 Files selected for processing (1)
  • slayer/storage/base.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • slayer/storage/base.py

📝 Walkthrough

Walkthrough

Adds a platform-aware default_storage_path() helper, uses it in the CLI to compute and show the storage default, and updates docs/README and MCP examples to remove explicit --storage ./slayer_data and document platform defaults and $SLAYER_STORAGE override.

Changes

Cohort / File(s) Summary
Docs — storage & examples
CLAUDE.md, docs/getting-started/mcp.md, docs/configuration/storage.md
Removed explicit --storage ./slayer_data from CLI examples and MCP JSON configs; added "Default Location" section documenting platform-specific default paths and $SLAYER_STORAGE override.
Docs — roadmap
README.md
Updated roadmap table: marked "Smart output formatting (currency, percentages)" as complete and moved "Unpivoting" to the next row.
Storage utility
slayer/storage/base.py
Added public default_storage_path() that resolves default storage via SLAYER_STORAGE, fallback to SLAYER_MODELS_DIR, then OS-specific base directories (XDG/~/.local/share or LOCALAPPDATA) and appends slayer.
CLI integration
slayer/cli.py
Replaced prior env-based default constant with call to default_storage_path() for help text interpolation and removed unused os import.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

  • whimo
  • ZmeiGorynych

Poem

🐰 I hop through folders neat and small,
I sniff the env and check them all.
No hardcoded trails to lose my way,
Defaults find home and brighten the day.
Hooray for paths that help me play! 🌿

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Default storage path' directly and concisely summarizes the main change: introducing a default storage path resolution mechanism across the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ 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-04-17-default-models-path

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/getting-started/mcp.md (1)

54-56: ⚠️ Potential issue | 🟡 Minor

Documentation references outdated default path.

Line 56 instructs users to create a file at slayer_data/datasources/mydb.yaml, but with the new default storage behavior, the actual default path is now platform-specific (e.g., ~/.local/share/slayer on Linux). This could confuse users who follow these instructions without using --storage.

Consider updating to reference the platform default or adding a note about the default location:

📝 Suggested update
-Create a file at `slayer_data/datasources/mydb.yaml`:
+Create a file in your storage folder's `datasources/` directory (e.g., `~/.local/share/slayer/datasources/mydb.yaml` on Linux — see [Storage Backends](../configuration/storage.md) for platform defaults):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/getting-started/mcp.md` around lines 54 - 56, The docs currently tell
users to create datasources at the hardcoded path
`slayer_data/datasources/mydb.yaml`, which is outdated; update the text in
docs/getting-started/mcp.md to mention the platform-specific default storage
location (e.g., "~/.local/share/slayer" on Linux) and/or explain that users can
override it with the `--storage` flag, replacing the explicit `slayer_data/...`
example with either a note about the default path per OS or showing both the
default path and an example when `--storage` is used so readers aren’t misled.
🧹 Nitpick comments (2)
CLAUDE.md (1)

85-85: Consider adding Windows default path for completeness.

The CLI documentation mentions Linux and macOS defaults but omits Windows (%LOCALAPPDATA%\slayer). Since the code in slayer/storage/base.py supports Windows, consider adding it here for completeness.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLAUDE.md` at line 85, Update the CLI docs sentence describing storage
defaults to include the Windows default path (%LOCALAPPDATA%\slayer), since the
implementation in slayer/storage/base.py already supports Windows; modify the
line mentioning Linux and macOS to also list Windows and ensure the env var and
legacy flag behavior description remains unchanged.
slayer/cli.py (1)

84-87: Epilog example still references explicit --storage ./slayer_data.

The example in the MCP command epilog suggests using --storage ./slayer_data, but since the default is now platform-appropriate, this example might mislead users into thinking they need to specify storage explicitly. Consider updating to show the simpler form or clarify when explicit storage is useful.

📝 Suggested update
   # Add to Claude Code:
-  claude mcp add slayer -- slayer mcp --storage ./slayer_data
+  claude mcp add slayer -- slayer mcp

Or if you want to show the override capability:

   # Add to Claude Code:
   claude mcp add slayer -- slayer mcp
+
+  # Or with explicit storage path:
+  claude mcp add slayer -- slayer mcp --storage ./my_custom_data
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@slayer/cli.py` around lines 84 - 87, Update the MCP command epilog string
that currently contains "claude mcp add slayer -- slayer mcp --storage
./slayer_data" to avoid suggesting an explicit --storage by default; edit the
MCP command epilog (the epilog string in slayer/cli.py where the "claude mcp add
slayer..." example is defined) to either show the simpler form without --storage
(e.g., "claude mcp add slayer -- slayer mcp") or include a brief parenthetical
clarifier that --storage is only needed to override the platform-default storage
path, so the example no longer implies it is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@docs/getting-started/mcp.md`:
- Around line 54-56: The docs currently tell users to create datasources at the
hardcoded path `slayer_data/datasources/mydb.yaml`, which is outdated; update
the text in docs/getting-started/mcp.md to mention the platform-specific default
storage location (e.g., "~/.local/share/slayer" on Linux) and/or explain that
users can override it with the `--storage` flag, replacing the explicit
`slayer_data/...` example with either a note about the default path per OS or
showing both the default path and an example when `--storage` is used so readers
aren’t misled.

---

Nitpick comments:
In `@CLAUDE.md`:
- Line 85: Update the CLI docs sentence describing storage defaults to include
the Windows default path (%LOCALAPPDATA%\slayer), since the implementation in
slayer/storage/base.py already supports Windows; modify the line mentioning
Linux and macOS to also list Windows and ensure the env var and legacy flag
behavior description remains unchanged.

In `@slayer/cli.py`:
- Around line 84-87: Update the MCP command epilog string that currently
contains "claude mcp add slayer -- slayer mcp --storage ./slayer_data" to avoid
suggesting an explicit --storage by default; edit the MCP command epilog (the
epilog string in slayer/cli.py where the "claude mcp add slayer..." example is
defined) to either show the simpler form without --storage (e.g., "claude mcp
add slayer -- slayer mcp") or include a brief parenthetical clarifier that
--storage is only needed to override the platform-default storage path, so the
example no longer implies it is required.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 80ea3a3a-ebbb-4ca9-9daa-415c3cb388e7

📥 Commits

Reviewing files that changed from the base of the PR and between cc1bad2 and fbc0a23.

📒 Files selected for processing (6)
  • CLAUDE.md
  • README.md
  • docs/configuration/storage.md
  • docs/getting-started/mcp.md
  • slayer/cli.py
  • slayer/storage/base.py

@AivanF AivanF force-pushed the aivan/2026-04-17-default-models-path branch from 577663e to 2db2674 Compare April 17, 2026 11:03
@AivanF AivanF merged commit 88fc639 into main Apr 17, 2026
3 checks passed
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.

1 participant