Skip to content

Add san cli#193

Open
tspenov wants to merge 6 commits intomasterfrom
add-san-cli
Open

Add san cli#193
tspenov wants to merge 6 commits intomasterfrom
add-san-cli

Conversation

@tspenov
Copy link
Copy Markdown
Collaborator

@tspenov tspenov commented Jan 26, 2026

   Usage: san [OPTIONS] COMMAND [ARGS]...

   Santiment API CLI - cryptocurrency data at your fingertips.

  ╭─ Options ────────────────────────────────────────────────────────────────────╮
  │ --version  -v        Show version                                            │
  │ --help               Show this message and exit.                             │
  ╰──────────────────────────────────────────────────────────────────────────────╯
  ╭─ Commands ───────────────────────────────────────────────────────────────────╮
  │ metrics      List available metrics.                                         │
  │ projects     List all available projects/assets.                             │
  │ get          Fetch timeseries data for a single metric/asset pair.           │
  │ get-many     Fetch timeseries data for a metric across multiple assets.      │
  │ rate-limit   Show API rate limit status (calls remaining).                   │
  │ api-calls    Show API calls history.                                         │
  │ complexity   Check query complexity for a metric.                            │
  │ config       Manage API configuration.                                       │
  ╰──────────────────────────────────────────────────────────────────────────────╯

Summary by CodeRabbit

  • New Features
    • Full command-line interface for Santiment interactions, including config, discovery (metrics/projects), data retrieval (single/multi-asset), and utilities (rate limits, API calls, complexity)
    • Output formats: table, JSON, CSV
  • Documentation
    • Added CLI usage and examples to the README
  • Chores
    • Package updated to expose the new CLI entry point and bumped version to 0.14.0

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 26, 2026

📝 Walkthrough

Walkthrough

Adds a new Typer-based CLI for sanpy with commands for configuration, discovery, data retrieval, and diagnostics; introduces cross-platform CLI config storage and output formatters; adds unit and integration tests; updates packaging to expose a san console script and bumps the package version.

Changes

Cohort / File(s) Summary
CLI Core
san/cli.py
New Typer app and subcommand groups: config (set-key, show, path, clear), metrics, projects, get, get-many, rate-limit, api-calls, complexity. Global options for --format, --api-key, --retries, --timeout, unified API key init and error handling, --version callback, and __main__ entrypoint.
CLI Config
san/cli_config.py
New cross-platform config helpers: determine config dir/path, read/write config.json, set/clear/get API key, mask API key for display, and ConfigError on filesystem/JSON failures.
CLI Formatters
san/cli_formatters.py
New output helpers: format_dataframe, format_list, format_dict, format_api_calls, and output supporting table/json/csv modes and handling pandas DataFrame formatting.
Tests
san/tests/test_cli.py, san/tests/test_cli_integration.py
New unit tests (CliRunner, mocked API calls) and integration tests (real API-backed invocations) covering help/version, all CLI commands, formats (table/json/csv), error handling, and global flags.
Packaging & Docs
setup.py, README.md
Bumps package version to 0.14.0, adds typer>=0.9.0 to install_requires, registers san console script entry point, and documents CLI usage and examples in README.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant CLI as "CLI (san/cli.py)"
    participant Config as "Config (san/cli_config.py)"
    participant API as "Santiment API (san.*)"
    participant Formatter as "Formatters (san/cli_formatters.py)"
    participant Output as "Output (stdout/stderr)"

    User->>CLI: Run command (e.g., `san get ...`)
    CLI->>Config: _init_api_key() -> load env/flag/stored key
    Config-->>CLI: api_key (or error)
    CLI->>API: Call API function (with api_key, retries, timeout)
    API-->>CLI: Response (DataFrame / list / dict) or raise SanError
    CLI->>Formatter: format_* (data, fmt)
    Formatter-->>CLI: formatted text
    CLI->>Output: output(text)
    Output-->>User: display result (table/json/csv)

    alt Error
        API-->>CLI: SanError / Exception
        CLI->>Output: output(error_text, err=True)
        Output-->>User: display error (stderr) and exit 1
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • createMonster
  • IvanIvanoff

Poem

"🐇 From the burrow I code and hop,
A CLI to fetch data non-stop.
Keys tucked safe and output neat,
JSON, CSV, or table to eat.
Hooray — the rabbit ships a clean CLI treat!"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Add san cli' is vague and generic. While it refers to a real part of the change (adding a CLI), it lacks specificity about what the CLI does or its purpose. Consider a more descriptive title such as 'Add Santiment API command-line interface' or 'Introduce san CLI for Santiment API data access' to better convey the main change.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 98.78% 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 add-san-cli

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown

@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.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@san/tests/test_cli_integration.py`:
- Around line 130-131: The test currently uses a weak check `assert "datetime"
in data[0] or "value" in data[0]`; update the assertion to require both fields
by changing it to `assert "datetime" in data[0] and "value" in data[0]` (i.e.,
validate that both keys exist on the first element of `data`), so the structure
check for `data[0]` verifies both expected fields are present.

In `@san/tests/test_cli.py`:
- Around line 24-29: The fixture mock_api_key is unused; either remove the
unused pytest fixture definition named mock_api_key or attach it to tests that
require it by adding mock_api_key as a test parameter; if keeping it, ensure the
patch target "san.ApiConfig" is correct and referenced by tests (e.g., include
mock_api_key in test function signatures that rely on san.ApiConfig.api_key)
otherwise delete the entire mock_api_key fixture block to eliminate dead code.
🧹 Nitpick comments (8)
san/cli_config.py (2)

24-32: Potential type inconsistency with Path conversion.

When APPDATA or XDG_CONFIG_HOME is not set, Path.home() returns a Path object, but when the env var exists, os.environ.get() returns a string. While Path(base) on line 31 handles this, it would be cleaner to ensure consistent types.

♻️ Suggested improvement
 def get_config_dir() -> Path:
     """Get the platform-appropriate config directory."""
     if os.name == "nt":  # Windows
-        base = os.environ.get("APPDATA", Path.home())
+        base = Path(os.environ.get("APPDATA", str(Path.home())))
     else:  # Linux/Mac
-        base = os.environ.get("XDG_CONFIG_HOME", Path.home() / ".config")
+        xdg_config = os.environ.get("XDG_CONFIG_HOME")
+        base = Path(xdg_config) if xdg_config else Path.home() / ".config"
 
-    config_dir = Path(base) / "sanpy"
+    config_dir = base / "sanpy"
     return config_dir

56-65: Silent error handling may hide config file corruption.

When _load_config encounters a JSONDecodeError, it silently returns an empty dict. This could mask a corrupted config file from the user. Consider logging a warning or informing the user that their config was reset.

♻️ Suggested improvement
 def _load_config() -> dict:
-    """Load config from file, return empty dict if doesn't exist."""
+    """Load config from file, return empty dict if doesn't exist or is corrupted."""
     config_path = get_config_path()
     if config_path.exists():
         try:
             with open(config_path, "r") as f:
                 return json.load(f)
-        except (json.JSONDecodeError, IOError):
+        except json.JSONDecodeError:
+            # Config file is corrupted, return empty and let next save overwrite
+            import sys
+            print(f"Warning: Config file '{config_path}' is corrupted, using defaults.", file=sys.stderr)
+            return {}
+        except IOError:
             return {}
     return {}
san/tests/test_cli_integration.py (1)

321-338: Consider adding assertion for error message content.

The test verifies the exit code and that "error" appears in stdout, but doesn't validate that the error is specifically about the invalid slug. This could help catch regressions in error messaging.

san/cli_formatters.py (2)

49-54: CSV and table formats produce identical output for lists.

The csv and table branches produce the same output ("\n".join(items)). For true CSV format, consider adding a header or quoting values. If this is intentional, a comment would clarify the design choice.


100-104: Inconsistent CSV escaping - consider using csv module.

format_dict uses the csv module for proper escaping, but format_api_calls uses string formatting. If a date string contains commas or special characters, this could produce malformed CSV.

♻️ Suggested fix
     elif fmt == "csv":
-        lines = ["date,api_calls"]
-        for date, count in calls:
-            lines.append(f"{date},{count}")
-        return "\n".join(lines)
+        buffer = io.StringIO()
+        writer = csv.writer(buffer, quoting=csv.QUOTE_MINIMAL)
+        writer.writerow(["date", "api_calls"])
+        for date, count in calls:
+            writer.writerow([date, count])
+        return buffer.getvalue()
san/cli.py (2)

72-78: Consider security implications of displaying config path.

Echoing the full config path after saving the API key is helpful for transparency, but ensure users understand the key is stored in plaintext.

♻️ Optional enhancement for security awareness
 def config_set_key(
     api_key: Annotated[str, typer.Argument(help="Your Santiment API key")],
 ) -> None:
     """Store API key in config file."""
     set_api_key(api_key)
-    output(f"API key saved to {get_config_path()}")
+    output(f"API key saved to {get_config_path()}")
+    output("Note: The API key is stored in plaintext.")

164-188: --slug is a required option - consider making it a positional argument.

The slug parameter is marked as required (no default value), which is correct. However, for better UX, consider whether slug should be a positional argument like metric, enabling san get price_usd bitcoin instead of san get price_usd --slug bitcoin.

setup.py (1)

19-19: Update the typer version constraint.

The typer>=0.9.0 constraint is significantly outdated. The latest stable version is 0.21.1 (released Jan 6, 2026). Consider updating the constraint to typer>=0.21.0 or higher to benefit from bug fixes, performance improvements, and new features from recent releases.

Copy link
Copy Markdown

@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.

♻️ Duplicate comments (1)
san/tests/test_cli.py (1)

24-29: 🛠️ Refactor suggestion | 🟠 Major

Fixture is unused and should be removed.

The mock_api_key fixture is defined but never referenced by any test function. This is dead code that should be removed.

♻️ Remove unused fixture
-@pytest.fixture
-def mock_api_key():
-    """Set up a mock API key for tests."""
-    with patch("san.ApiConfig") as mock_config:
-        mock_config.api_key = "test_api_key_12345"
-        yield mock_config
-
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@san/tests/test_cli.py` around lines 24 - 29, Remove the unused pytest fixture
by deleting the mock_api_key function definition (the pytest.fixture block named
mock_api_key) from the test file; also remove any now-unused imports (e.g.,
patch or other mocking helpers) that existed solely to support mock_api_key so
there are no leftover unused imports or dead code.
🧹 Nitpick comments (3)
san/cli_config.py (2)

114-119: Config file persists even when empty after clearing.

After clearing the API key, if no other config keys exist, the file will contain just {}. This is fine, but consider whether you want to delete the file entirely when it becomes empty.

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

In `@san/cli_config.py` around lines 114 - 119, clear_api_key currently deletes
"api_key" and writes an empty dict back via _save_config, leaving a {} file;
change clear_api_key to call _load_config, delete "api_key", then if the
resulting config is empty (len(config) == 0) remove the config file instead of
calling _save_config: either call an existing helper to delete the config file
or add a small helper (e.g., _remove_config_file) and invoke it from
clear_api_key; otherwise call _save_config as before. Ensure you reference the
existing helpers _load_config and _save_config and the function name
clear_api_key when making the change.

56-65: Silent failure on corrupted config may mask issues.

When json.JSONDecodeError is caught, the function silently returns an empty dict. While this prevents crashes, the user won't know their config file is corrupted. Consider logging a warning or informing the user.

♻️ Optional: Add warning for corrupted config
 def _load_config() -> dict:
     """Load config from file, return empty dict if doesn't exist."""
     config_path = get_config_path()
     if config_path.exists():
         try:
             with open(config_path, "r") as f:
                 return json.load(f)
-        except (json.JSONDecodeError, IOError):
+        except json.JSONDecodeError:
+            import warnings
+            warnings.warn(f"Config file '{config_path}' is corrupted, using defaults")
+            return {}
+        except IOError:
             return {}
     return {}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@san/cli_config.py` around lines 56 - 65, The _load_config function currently
swallows json.JSONDecodeError and returns {} silently; update it to catch
JSONDecodeError separately and emit a warning that includes the config file path
(from get_config_path()/config_path) and the error details before returning an
empty dict so users know their config is corrupted; use the module logger (e.g.
logging.getLogger(__name__)) or a clear print call and keep IOError handled as
before.
san/cli_formatters.py (1)

27-31: DataFrame mutation may affect caller's data.

The df.reset_index() result is assigned back to df, but the original df passed in may still be mutated if it's a view. While reset_index() returns a new DataFrame by default, this reassignment shadows the parameter, which is fine. However, for absolute safety and clarity, consider making an explicit copy if the DataFrame might be reused.

     if fmt == "json":
         # Reset index to include datetime in output
         if df.index.name == "datetime" or isinstance(df.index, pd.DatetimeIndex):
-            df = df.reset_index()
+            df = df.reset_index()  # Creates a new DataFrame, original is unchanged
         return df.to_json(orient="records", date_format="iso", indent=2)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@san/cli_formatters.py` around lines 27 - 31, In the fmt == "json" block avoid
mutating the caller's DataFrame by operating on an explicit copy: create a local
copy of df (e.g., df_copy = df.copy()) before calling reset_index() and
to_json(), then use that copy for any reset_index()/to_json() calls so the
original df passed in is never altered; update references to df in this block
(reset_index and to_json) to use the new local copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@san/tests/test_cli.py`:
- Around line 24-29: Remove the unused pytest fixture by deleting the
mock_api_key function definition (the pytest.fixture block named mock_api_key)
from the test file; also remove any now-unused imports (e.g., patch or other
mocking helpers) that existed solely to support mock_api_key so there are no
leftover unused imports or dead code.

---

Nitpick comments:
In `@san/cli_config.py`:
- Around line 114-119: clear_api_key currently deletes "api_key" and writes an
empty dict back via _save_config, leaving a {} file; change clear_api_key to
call _load_config, delete "api_key", then if the resulting config is empty
(len(config) == 0) remove the config file instead of calling _save_config:
either call an existing helper to delete the config file or add a small helper
(e.g., _remove_config_file) and invoke it from clear_api_key; otherwise call
_save_config as before. Ensure you reference the existing helpers _load_config
and _save_config and the function name clear_api_key when making the change.
- Around line 56-65: The _load_config function currently swallows
json.JSONDecodeError and returns {} silently; update it to catch JSONDecodeError
separately and emit a warning that includes the config file path (from
get_config_path()/config_path) and the error details before returning an empty
dict so users know their config is corrupted; use the module logger (e.g.
logging.getLogger(__name__)) or a clear print call and keep IOError handled as
before.

In `@san/cli_formatters.py`:
- Around line 27-31: In the fmt == "json" block avoid mutating the caller's
DataFrame by operating on an explicit copy: create a local copy of df (e.g.,
df_copy = df.copy()) before calling reset_index() and to_json(), then use that
copy for any reset_index()/to_json() calls so the original df passed in is never
altered; update references to df in this block (reset_index and to_json) to use
the new local copy.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1ca20c81-8cba-43b6-86b6-90856b8c036b

📥 Commits

Reviewing files that changed from the base of the PR and between d9b378f and 8676016.

📒 Files selected for processing (5)
  • san/cli.py
  • san/cli_config.py
  • san/cli_formatters.py
  • san/tests/test_cli.py
  • san/tests/test_cli_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • san/tests/test_cli_integration.py

Copy link
Copy Markdown

@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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@san/cli.py`:
- Around line 230-233: After parsing slugs into slug_list (from the slugs
variable) add a guard that validates slug_list is not empty before building
kwargs and calling san.get_many; if empty, surface a clear, immediate error
(e.g., raise a user-facing exception or exit with an error message) so the CLI
fails fast with an actionable message instead of allowing an empty list to
propagate into san.get_many; update the code around
slug_list/kwargs/san.get_many to perform this check and return/exit on failure.
- Around line 85-90: The CLI currently accepts the API key as a positional
argument in config_set_key which can expose secrets; change the command to
obtain the key securely (e.g., convert the api_key parameter from a positional
typer.Argument to a typed option/flag or remove the argument and prompt
interactively) and use Typer's prompt with hide_input=True or read from
STDIN/ENV before calling set_api_key; update config_set_key to call
set_api_key(api_key) and preserve the confirmation output using
get_config_path() after securely acquiring the value.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4e83213e-feef-477e-9b99-13c9a4cd47cc

📥 Commits

Reviewing files that changed from the base of the PR and between 8676016 and c57173a.

📒 Files selected for processing (5)
  • san/cli.py
  • san/cli_config.py
  • san/cli_formatters.py
  • san/tests/test_cli.py
  • san/tests/test_cli_integration.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • san/cli_config.py
  • san/cli_formatters.py

Copy link
Copy Markdown

@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.

Actionable comments posted: 3

♻️ Duplicate comments (2)
san/cli.py (2)

229-233: ⚠️ Potential issue | 🟡 Minor

Reject an empty parsed --slugs list.

Inputs like --slugs "," currently become [] and flow into san.get_many(...), so the user gets a downstream error instead of an actionable CLI validation message.

Proposed fix
 def get_many(
@@
 ) -> None:
     """Fetch timeseries data for a metric across multiple assets."""
     _init_api_key(api_key)
+    slug_list = [s.strip() for s in slugs.split(",") if s.strip()]
+    if not slug_list:
+        raise typer.BadParameter("Provide at least one valid slug in --slugs.")
     try:
-        slug_list = [s.strip() for s in slugs.split(",") if s.strip()]
         kwargs = dict(
             slugs=slug_list,
             from_date=from_date,
             to_date=to_date,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@san/cli.py` around lines 229 - 233, After parsing slugs into slug_list,
validate that slug_list is non-empty and if it is empty (e.g., user passed
"--slugs ','" or only commas) raise a CLI-friendly error instead of letting an
empty list flow into san.get_many; update the parsing block that creates
slug_list (variable slug_list from input slugs) to detect an empty result and
raise a click.BadParameter (or call sys.exit with a clear message) indicating
the --slugs value is invalid so the user gets an actionable validation message.

84-90: ⚠️ Potential issue | 🟠 Major

Don't take the API key as a positional argument.

This leaks the secret via shell history and process inspection. Use a hidden prompt or another non-argv input path instead; the README/tests will need the same interface change.

Proposed hardening
 `@config_app.command`("set-key")
 def config_set_key(
-    api_key: Annotated[str, typer.Argument(help="Your Santiment API key")],
+    api_key: Annotated[
+        Optional[str],
+        typer.Option(
+            "--api-key",
+            prompt=True,
+            hide_input=True,
+            help="Your Santiment API key",
+        ),
+    ] = None,
 ) -> None:
     """Store API key in config file."""
+    if not api_key:
+        raise typer.BadParameter("API key is required.")
     set_api_key(api_key)
     output(f"API key saved to {get_config_path()}")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@san/cli.py` around lines 84 - 90, The command currently takes the API key as
a positional argument (config_set_key) which leaks secrets; change the api_key
parameter from an Argument to an Option that prompts hidden input (e.g.,
Annotated[str, typer.Option(..., prompt=True, hide_input=True)]) or
alternatively read from getpass when no tty is available, then call
set_api_key(api_key) and output the config path as before; update any
README/tests that expect a positional arg to use the new interactive/hidden
input or provide the option flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Around line 69-72: The fenced code block containing the commands "san --help"
and "san --version" lacks a language tag, which trips markdownlint and prevents
shell highlighting; update that block by changing the opening backticks to
include a language identifier (e.g., replace ``` with ```bash) so the block
reads ```bash followed by the two commands and closing ``` to enable shell
syntax highlighting.
- Line 111: The README currently claims "All commands support `--format`
(`table`, `json`, `csv`) and `--api-key` options" which is inaccurate; update
that sentence to scope the flag support to only the commands that `san/cli.py`
actually registers: the discovery, data, and diagnostic subcommands. Replace the
line with wording that explicitly states that `--format` and `--api-key` are
supported by the discovery, data, and diagnostic commands (and not by config
subcommands), referencing `san/cli.py` and the discovery/data/diagnostic command
names to make the scope clear.

In `@san/tests/test_cli.py`:
- Around line 105-115: The test currently asserts a generic "test" substring
which can collide with tmp path names; update test_config_show to assert the
masked token specifically by storing the key (e.g., key = "test_key_abcd1234"),
using runner.invoke(app, ["config", "set-key", key]) and then after
runner.invoke(app, ["config", "show"]) assert that the output contains the key's
visible prefix (e.g., key[:4] or the exact expected visible prefix) and that the
full key string is NOT present in result.stdout (or that the masked output
contains a masking character like '*' after the prefix), ensuring you reference
test_config_show, the same key variable, and the two runner.invoke calls when
making the change.

---

Duplicate comments:
In `@san/cli.py`:
- Around line 229-233: After parsing slugs into slug_list, validate that
slug_list is non-empty and if it is empty (e.g., user passed "--slugs ','" or
only commas) raise a CLI-friendly error instead of letting an empty list flow
into san.get_many; update the parsing block that creates slug_list (variable
slug_list from input slugs) to detect an empty result and raise a
click.BadParameter (or call sys.exit with a clear message) indicating the
--slugs value is invalid so the user gets an actionable validation message.
- Around line 84-90: The command currently takes the API key as a positional
argument (config_set_key) which leaks secrets; change the api_key parameter from
an Argument to an Option that prompts hidden input (e.g., Annotated[str,
typer.Option(..., prompt=True, hide_input=True)]) or alternatively read from
getpass when no tty is available, then call set_api_key(api_key) and output the
config path as before; update any README/tests that expect a positional arg to
use the new interactive/hidden input or provide the option flag.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b521df6c-39fc-4d09-b8c0-a8abad8b5788

📥 Commits

Reviewing files that changed from the base of the PR and between c57173a and 2f776fa.

📒 Files selected for processing (8)
  • README.md
  • examples/extras/event_study.ipynb
  • san/cli.py
  • san/cli_config.py
  • san/cli_formatters.py
  • san/tests/test_cli.py
  • san/tests/test_cli_integration.py
  • setup.py
✅ Files skipped from review due to trivial changes (1)
  • san/cli_config.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • setup.py
  • san/tests/test_cli_integration.py
  • san/cli_formatters.py
👮 Files not reviewed due to content moderation or server errors (1)
  • examples/extras/event_study.ipynb

Comment on lines +69 to +72
```
san --help
san --version
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add a language to this fenced block.

This trips markdownlint and loses shell highlighting.

Proposed fix
-```
+```bash
 san --help
 san --version
</details>

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>

[warning] 69-69: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @README.md around lines 69 - 72, The fenced code block containing the
commands "san --help" and "san --version" lacks a language tag, which trips
markdownlint and prevents shell highlighting; update that block by changing the
opening backticks to include a language identifier (e.g., replace withbash) so the block reads bash followed by the two commands and closing
to enable shell syntax highlighting.


</details>

<!-- fingerprinting:phantom:medusa:grasshopper -->

<!-- This is an auto-generated comment by CodeRabbit -->

san complexity price_usd # Query complexity check
```

All commands support `--format` (`table`, `json`, `csv`) and `--api-key` options.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Scope the flag-support claim more narrowly.

san/cli.py only adds --format/--api-key to the discovery/data/diagnostic commands. config subcommands don't accept those flags, so this sentence currently overstates the interface.

Proposed wording
-All commands support `--format` (`table`, `json`, `csv`) and `--api-key` options.
+The discovery, data, and diagnostics commands support `--format` (`table`, `json`, `csv`).
+API-backed commands also accept `--api-key`.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
All commands support `--format` (`table`, `json`, `csv`) and `--api-key` options.
The discovery, data, and diagnostics commands support `--format` (`table`, `json`, `csv`).
API-backed commands also accept `--api-key`.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 111, The README currently claims "All commands support
`--format` (`table`, `json`, `csv`) and `--api-key` options" which is
inaccurate; update that sentence to scope the flag support to only the commands
that `san/cli.py` actually registers: the discovery, data, and diagnostic
subcommands. Replace the line with wording that explicitly states that
`--format` and `--api-key` are supported by the discovery, data, and diagnostic
commands (and not by config subcommands), referencing `san/cli.py` and the
discovery/data/diagnostic command names to make the scope clear.

Comment on lines +105 to +115
def test_config_show(tmp_path, monkeypatch):
"""Test showing config."""
monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path))

# First set a key
runner.invoke(app, ["config", "set-key", "test_key_abcd1234"])

# Then show config
result = runner.invoke(app, ["config", "show"])
assert result.exit_code == 0
assert "test" in result.stdout # Masked key shows first chars
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Assert the masked token, not a generic test substring.

This can pass even if masking regresses, because config show also prints a path under tmp_path, and pytest temp directories typically include test_....

Proposed fix
 def test_config_show(tmp_path, monkeypatch):
     """Test showing config."""
     monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path))
 
     # First set a key
-    runner.invoke(app, ["config", "set-key", "test_key_abcd1234"])
+    set_result = runner.invoke(app, ["config", "set-key", "test_key_abcd1234"])
+    assert set_result.exit_code == 0
 
     # Then show config
     result = runner.invoke(app, ["config", "show"])
     assert result.exit_code == 0
-    assert "test" in result.stdout  # Masked key shows first chars
+    assert "Stored API key: test...1234" in result.stdout
+    assert "test_key_abcd1234" not in result.stdout
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@san/tests/test_cli.py` around lines 105 - 115, The test currently asserts a
generic "test" substring which can collide with tmp path names; update
test_config_show to assert the masked token specifically by storing the key
(e.g., key = "test_key_abcd1234"), using runner.invoke(app, ["config",
"set-key", key]) and then after runner.invoke(app, ["config", "show"]) assert
that the output contains the key's visible prefix (e.g., key[:4] or the exact
expected visible prefix) and that the full key string is NOT present in
result.stdout (or that the masked output contains a masking character like '*'
after the prefix), ensuring you reference test_config_show, the same key
variable, and the two runner.invoke calls when making the change.

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