Repository navigation
feat: implement technical improvements from senior review - #13
Conversation
Co-Authored-By: martyy-code <nesalia.inc@gmail.com>
Add reddit_cli/logging.py with: - Python logging module configuration - Log levels: DEBUG, INFO, WARNING, ERROR - Log to stderr - Reusable logger instance for the codebase
Replace typer.echo for errors with proper logging calls: - Use logger.error for API errors - Use logger.warning for rate limiting and validation errors - Use logger.info for interrupts - Keep user-friendly messages via typer.echo for final output
Add rich library to dependencies for enhanced CLI output formatting
Create reddit_cli/ui.py with: - print_table: Print data as a Rich table - print_posts: Pretty print posts with colored scores - print_comments: Print comments with indentation - print_progress: Show progress during loading
…display Replace plain text post display with Rich-powered print_posts: - browse: Use Rich table for post listings - search: Use Rich table for post listings - navigation: Use Rich table for frontpage/home/best posts
Replace plain text subreddit listings with Rich-powered tables: - subreddits popular: Use Rich table for listings - subreddits search: Use Rich table with NSFW tags - subreddits new/gold/default: Use Rich table for listings
Create tests/test_async.py with tests for: - RedditClient context manager lifecycle - Basic GET request functionality - Retry on rate limit (429) - Max retries exhaustion on 429 - Server error (500) retries - Successful request without retries - Timeout and connection error handling
Create reddit_cli/cache.py with: - JSON file-based cache in ~/.cache/reddit_cli/ - Cache TTL of 5 minutes for listings - Cache key based on (endpoint, params) - get_cached/set_cached/clear_cache functions - get_cache_size for cache statistics
Add _validate_table_name() function that whitelist-validates table names before using them in SQL INSERT statements. Table names must match ^[a-zA-Z_][a-zA-Z0-9_]*$ pattern and be at most 64 characters. Applied to post_to_sql_insert, comment_to_sql_insert, and subreddit_to_sql_insert functions. Co-Authored-By: martyy-code <nesalia.inc@gmail.com>
- Create reddit_cli/commands/_shared.py with common constants and functions:
- VALID_FORMAT_VALUES, VALID_SORT_VALUES, VALID_PERIOD_VALUES
- VALID_SEARCH_SORT_VALUES, VALID_SEARCH_PERIOD_VALUES
- VALID_SUBREDDIT_SORT_VALUES
- _validate_sort_period() - generic validation function
- _validate_format() - format validation helper
- validate_output_path() - path traversal prevention
- Add path validation for --output option in all command files:
- browse.py, search.py, comments.py, subreddit.py, post.py
- Prevents path traversal attacks (checks for ".." and "-")
- Returns validated Path object or raises typer.Exit(code=2)
- Update tests/conftest.py:
- Add mock_reddit_base_strict fixture for tests that need
to verify all mocked endpoints were called
Co-Authored-By: martyy-code <nesalia.inc@gmail.com>
Co-Authored-By: martyy-code <nesalia.inc@gmail.com>
Co-Authored-By: martyy-code <nesalia.inc@gmail.com>
Co-Authored-By: martyy-code <nesalia.inc@gmail.com>
- Fix test_search_format_display to look for '500' instead of '[500]' - Increase Rich console width to 120 to prevent text wrapping - Remove title truncation to preserve full post titles
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds pre-commit hooks and project config updates; introduces structured logging, a file-based cache, and Rich UI helpers; centralizes CLI validation and output-path checks; extends export formats and SQL table validation; refactors command modules to use shared utilities; and adds/adjusts tests including async client coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
.pre-commit-config.yaml (1)
14-18: Consider making the test hook optional or faster.Running the full test suite on every commit can slow down the development workflow. Consider:
- Moving this to a
stages: [pre-push]hook instead, or- Adding
pass_filenames: falsesince the hook ignores staged files anyway.Also, the
mypyhook (lines 9-13) should havepass_filenames: falsesince it targets a fixed directory.Suggested improvement
- id: typecheck name: mypy type check entry: mypy reddit_cli language: system types: [python] + pass_filenames: false - id: test name: pytest entry: pytest tests/ -x -q language: system types: [python] + pass_filenames: false + stages: [pre-push]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 14 - 18, The pre-commit config runs the full pytest hook ("id: test" / "name: pytest") on every commit which slows workflows; either move that hook to run pre-push by adding stages: [pre-push] to the pytest hook, or mark it as non-file-dependent by adding pass_filenames: false to the "test" hook so it doesn't run per-staged-file; also add pass_filenames: false to the mypy hook (the mypy hook targeting a fixed directory) so mypy runs consistently rather than being invoked with staged filenames.reddit_cli/export.py (1)
33-43: SQL value escaping is incomplete - consider using parameterized output.The
escape_sql_valuefunction doesn't handle NULL bytes (\x00) which can bypass escaping in some databases. While the PR focuses on table name validation, the value escaping remains a secondary risk.For generated SQL intended for import, consider either:
- Adding
\x00to the escape chain- Documenting that output should be reviewed before execution
Minimal fix for NULL byte handling
def escape_sql_value(value: str) -> str: - return value.replace("\\", "\\\\").replace("'", "\\'").replace("\n", "\\n").replace("\r", "\\r") + return value.replace("\\", "\\\\").replace("\x00", "").replace("'", "\\'").replace("\n", "\\n").replace("\r", "\\r")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@reddit_cli/export.py` around lines 33 - 43, The escape_sql_value function fails to escape NULL bytes (\x00), which can bypass escaping; update escape_sql_value to also replace "\x00" (e.g., .replace("\x00", "\\0") or a chosen safe sentinel) in the same replacement chain and add a brief comment in the function docstring recommending use of parameterized queries for generated SQL to avoid injection risks; target the escape_sql_value function for this change.reddit_cli/cache.py (1)
104-109: Silent failure on cache write may hide persistent issues.Silently passing on
OSErroris reasonable for a cache, but consider logging a debug message to aid troubleshooting disk permission or space issues.Optional: Add debug logging
try: with open(cache_file, "w") as f: json.dump(cache_data, f) except OSError: - # Silently fail if we can't write to cache - pass + # Cache write failures are non-fatal but worth logging for debugging + import logging + logging.getLogger("reddit_cli").debug("Failed to write cache file: %s", cache_file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@reddit_cli/cache.py` around lines 104 - 109, The except block that currently swallows OSError when writing the cache should log the failure for debugging: change "except OSError:" to "except OSError as e:" and emit a debug-level log including cache_file and the exception details (e.g., logger.debug or logging.debug with a message like "failed to write cache %s: %s", cache_file, e) so we still tolerate write failures but surface permission/space errors for troubleshooting; keep the silent-fail behavior otherwise.pyproject.toml (1)
28-32: Hardcoded version in towncrier config requires manual sync.The
version = "0.6.0"duplicates the version from line 3. This creates a maintenance burden as both need updating on each release. Consider using dynamic versioning or a single source of truth.Note:
httpxappears in both main dependencies (viatyper/pydanticusage) and dev dependencies (line 10). If it's already a runtime dependency, it can be removed from dev dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 28 - 32, The towncrier config currently hardcodes version = "0.6.0" causing duplicate version state; change the [tool.towncrier] block to pull the project version from a single source of truth instead of a literal string (e.g., use towncrier's dynamic/version-from-project mechanism such as setting version to the project metadata reference instead of "0.6.0") so the package version in [project] is authoritative; also remove the duplicate httpx entry from dev-dependencies if httpx is already included as a runtime dependency via your main dependencies (typer/pydantic) to avoid redundant listings.tests/test_async.py (1)
66-87: Stub out retry backoff in the unit tests.
RedditClient.get()inreddit_cli/reddit/base.pysleeps for 1s and then 2s on the retrying 429/500 paths, so these tests add about 9 seconds of real waiting to every run. Monkeypatchreddit_cli.reddit.base.asyncio.sleepand assert the awaited delays instead.Suggested pattern
+from unittest.mock import AsyncMock, call ... `@pytest.mark.asyncio` `@respx.mock` -async def test_reddit_client_retry_on_429(): +async def test_reddit_client_retry_on_429(monkeypatch): """Test that client retries on rate limit (429) with exponential backoff.""" + sleep = AsyncMock() + monkeypatch.setattr("reddit_cli.reddit.base.asyncio.sleep", sleep) + # First two requests return 429, third succeeds mock_response = { "data": { "children": [], "after": None, @@ async with RedditClient() as client: result = await client.get("/r/python/hot.json") assert result == mock_response assert route.call_count == 3 + sleep.assert_has_awaits([call(1.0), call(2.0)])Also applies to: 92-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_async.py` around lines 66 - 87, The test is causing real sleeps because RedditClient.get in reddit_cli.reddit.base calls asyncio.sleep on retries; stub out and assert those sleeps by monkeypatching reddit_cli.reddit.base.asyncio.sleep with an async spy in test_reddit_client_retry_on_429 (and the similar test at lines 92-113) so the test runs instantly and verifies the backoff timing was awaited; locate the call site via the RedditClient.get method and replace asyncio.sleep with an async function that records its awaited durations (or a pytest-mock async spy) and assert it was called with 1 and then 2 seconds while keeping the mocked HTTP responses as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.husky/pre-commit:
- Around line 1-14: Add "set -e" (and optionally "set -o pipefail") immediately
after the shebang in the pre-commit shell script so the script fails fast on any
ruff, mypy, or pytest error; update the top of the script (the line after "#
Pre-commit hook for reddit-cli") to include these options. Also consider
removing the duplicated checks from .pre-commit-config.yaml or the husky script
and centralize hooks in one mechanism (either husky or pre-commit) to avoid
maintenance duplication.
In `@reddit_cli/__init__.py`:
- Around line 125-129: The usage examples for the completion command are
incorrect: the CLI defines the shell parameter as a Typer option named --shell
(see the typer.Option for shell around the completion command), so update the
four example lines to call the command with the flag (e.g. "reddit completion
--shell bash >> ~/.bashrc", "reddit completion --shell zsh >> ~/.zshrc", "reddit
completion --shell fish > ~/.config/fish/completions/reddit.fish", "reddit
completion --shell powershell >> $PROFILE") so they match the completion
function's --shell parameter and include the proper redirections.
- Around line 131-149: Remove the invalid and unused import "from typer import
complete" and change the literal-only f-strings in the completion/help block to
plain string literals: in the function handling the "shell" argument (where
prog_name = "reddit" is set), replace the import removal and update the
typer.echo calls that currently use f-strings with no interpolations (the lines
that print "Run this command to enable ... completion:" and the example commands
for bash, zsh, fish, and powershell) to use regular quoted strings (e.g., "Run
this command to enable bash completion:" and " eval \"$(reddit
--show-completion bash)\"") so Ruff F541 is resolved and the unused import is
eliminated.
In `@reddit_cli/cache.py`:
- Around line 4-6: Remove the unused imports in reddit_cli/cache.py: delete the
unused "import os" and the "from typing import Any" import (or if Any is
intended for a future annotation, replace its usage accordingly); keep "from
pathlib import Path" which is used by functions/classes in this module (e.g.,
any functions referencing Path) so the file no longer triggers the pipeline
warning about unused imports.
In `@reddit_cli/commands/_shared.py`:
- Around line 90-97: The current path traversal check is ineffective because
resolved = path.resolve() removes '..' components so path_str will never contain
".."; instead, either (A) check the original input string (e.g., if ".." in
str(path) or inspect path.parts for "..") before calling path.resolve(), or (B)
resolve and then ensure the resolved path remains inside an allowed base
directory (use resolved.is_relative_to(base_dir) on Python 3.9+ or compare
commonpath with os.path.commonpath) to enforce no traversal; update the code
that uses path, resolved and path_str accordingly to implement one of these
approaches.
- Line 13: Add "json" to the global VALID_FORMAT_VALUES list in _shared.py and
update the two calls to _validate_format() in comments.py to pass the module's
VALID_FORMAT_VALUES so JSON becomes an accepted format; specifically, edit
VALID_FORMAT_VALUES = ["display", "sql", "csv", "xlsx"] in _shared.py to include
"json" and change the _validate_format(...) invocations in comments.py (the
calls around where format is validated) to supply the comments.py
VALID_FORMAT_VALUES constant as the second argument to _validate_format().
In `@reddit_cli/commands/comments.py`:
- Around line 139-145: The call to _validate_format(format) is using the shared
default formats and must be passed the comments-specific allowed formats so the
"json" branch becomes reachable; update both occurrences (the one around
output_path and the later block) to call _validate_format(format,
COMMENTS_OUTPUT_FORMATS) (or the actual comments-specific format constant used
in the file) so the validator checks the comments command formats (allowing
"json") before proceeding.
In `@reddit_cli/commands/navigation.py`:
- Around line 88-89: The Rich list view lost post IDs because the navigation
command now calls print_posts(posts) which only renders
score/title/subreddit/author/comments; update the rendering to include the post
ID so users can reference it for commands like `reddit post` or `reddit
comments`. Modify reddit_cli.ui.print_posts (or the caller in
reddit_cli.commands.navigation where print_posts is invoked) to include the
post.id/ID in the displayed row (e.g., prepend or add an ID column) and ensure
the ID value is passed through from the posts data structure so the
frontpage/home/best output contains the stable identifier.
In `@reddit_cli/logging.py`:
- Around line 15-32: The CLI never initializes the logging config because
setup_logging() in logging.py is not invoked at startup; update the CLI
entrypoint (the __main__ block that calls app() in reddit_cli/__init__.py) to
import and call setup_logging() (with the desired level) before calling app() so
get_logger() in reddit_cli/errors.py and elsewhere will use the configured
basicConfig (timestamped format and provided handlers) rather than Python's
default logging; ensure you reference the setup_logging function and call it
prior to any logger usage or app() invocation.
In `@reddit_cli/ui.py`:
- Around line 8-10: The current shared Console named console is created with
stderr=True and is used for normal CLI output (print_table, print_posts,
print_comments) which should go to stdout; create two consoles instead (e.g.,
out_console = Console(stderr=False, width=120) for normal/tabular output and
err_console = Console(stderr=True, width=120) for progress/status), replace
usages of console in print_table, print_posts, print_comments to use
out_console, and update print_progress to write to err_console (and any other
status/logging helpers to use err_console) so piping/redirecting works as
expected.
- Around line 29-30: Sanitize all user-provided Reddit strings before passing
them to Rich to avoid markup interpretation: wrap values passed into
table.add_row (the loop that builds rows using columns and data) and any
console.print calls that render titles, subreddit names, authors, or comment
bodies with rich.markup.escape() so you pass escaped strings instead of raw
values; update the data->row creation (the list comprehension feeding
table.add_row) and the blocks that call console.print(...) to call
rich.markup.escape on each user-originated string (e.g., title, author,
subreddit, body) before formatting/printing.
In `@tests/test_async.py`:
- Around line 10-24: The test currently doesn't verify teardown or re-entry on
the same RedditClient instance; update test_reddit_client_context_manager to
reuse the same RedditClient object: after the first async with RedditClient() as
client block assert that client._client is None to confirm __aexit__ cleaned up,
then perform a second `async with client as client2` (re-enter the same
instance) to ensure __aenter__ reinitializes the client and then again assert
client._client is None after exit; reference the RedditClient class and its
__aenter__/__aexit__ behavior and the _client attribute to locate where to
change the test.
---
Nitpick comments:
In @.pre-commit-config.yaml:
- Around line 14-18: The pre-commit config runs the full pytest hook ("id: test"
/ "name: pytest") on every commit which slows workflows; either move that hook
to run pre-push by adding stages: [pre-push] to the pytest hook, or mark it as
non-file-dependent by adding pass_filenames: false to the "test" hook so it
doesn't run per-staged-file; also add pass_filenames: false to the mypy hook
(the mypy hook targeting a fixed directory) so mypy runs consistently rather
than being invoked with staged filenames.
In `@pyproject.toml`:
- Around line 28-32: The towncrier config currently hardcodes version = "0.6.0"
causing duplicate version state; change the [tool.towncrier] block to pull the
project version from a single source of truth instead of a literal string (e.g.,
use towncrier's dynamic/version-from-project mechanism such as setting version
to the project metadata reference instead of "0.6.0") so the package version in
[project] is authoritative; also remove the duplicate httpx entry from
dev-dependencies if httpx is already included as a runtime dependency via your
main dependencies (typer/pydantic) to avoid redundant listings.
In `@reddit_cli/cache.py`:
- Around line 104-109: The except block that currently swallows OSError when
writing the cache should log the failure for debugging: change "except OSError:"
to "except OSError as e:" and emit a debug-level log including cache_file and
the exception details (e.g., logger.debug or logging.debug with a message like
"failed to write cache %s: %s", cache_file, e) so we still tolerate write
failures but surface permission/space errors for troubleshooting; keep the
silent-fail behavior otherwise.
In `@reddit_cli/export.py`:
- Around line 33-43: The escape_sql_value function fails to escape NULL bytes
(\x00), which can bypass escaping; update escape_sql_value to also replace
"\x00" (e.g., .replace("\x00", "\\0") or a chosen safe sentinel) in the same
replacement chain and add a brief comment in the function docstring recommending
use of parameterized queries for generated SQL to avoid injection risks; target
the escape_sql_value function for this change.
In `@tests/test_async.py`:
- Around line 66-87: The test is causing real sleeps because RedditClient.get in
reddit_cli.reddit.base calls asyncio.sleep on retries; stub out and assert those
sleeps by monkeypatching reddit_cli.reddit.base.asyncio.sleep with an async spy
in test_reddit_client_retry_on_429 (and the similar test at lines 92-113) so the
test runs instantly and verifies the backoff timing was awaited; locate the call
site via the RedditClient.get method and replace asyncio.sleep with an async
function that records its awaited durations (or a pytest-mock async spy) and
assert it was called with 1 and then 2 seconds while keeping the mocked HTTP
responses as-is.
🪄 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: dcd44905-ee49-4aa3-a8e2-b2a7daebae73
📒 Files selected for processing (20)
.husky/pre-commit.pre-commit-config.yamlpyproject.tomlreddit_cli/__init__.pyreddit_cli/cache.pyreddit_cli/commands/_shared.pyreddit_cli/commands/browse.pyreddit_cli/commands/comments.pyreddit_cli/commands/navigation.pyreddit_cli/commands/post.pyreddit_cli/commands/search.pyreddit_cli/commands/subreddit.pyreddit_cli/errors.pyreddit_cli/export.pyreddit_cli/logging.pyreddit_cli/ui.pytests/conftest.pytests/test_async.pytests/test_cli_search.pytests/test_export.py
💤 Files with no reviewable changes (1)
- tests/test_export.py
| #!/bin/sh | ||
| # Pre-commit hook for reddit-cli | ||
|
|
||
| # Run ruff linting | ||
| echo "Running ruff lint..." | ||
| ruff check reddit_cli tests | ||
|
|
||
| # Run mypy type checking | ||
| echo "Running mypy type check..." | ||
| mypy reddit_cli | ||
|
|
||
| # Run pytest | ||
| echo "Running pytest..." | ||
| pytest tests/ -x -q |
There was a problem hiding this comment.
Add set -e to fail fast on errors.
Without set -e, the script continues after ruff or mypy failures, and only the final pytest exit code determines success. This could allow commits with lint or type errors.
Proposed fix
#!/bin/sh
# Pre-commit hook for reddit-cli
+set -e
# Run ruff lintingAdditionally, this duplicates the hooks in .pre-commit-config.yaml. Consider using only one mechanism (either husky or pre-commit) to reduce maintenance burden.
📝 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.
| #!/bin/sh | |
| # Pre-commit hook for reddit-cli | |
| # Run ruff linting | |
| echo "Running ruff lint..." | |
| ruff check reddit_cli tests | |
| # Run mypy type checking | |
| echo "Running mypy type check..." | |
| mypy reddit_cli | |
| # Run pytest | |
| echo "Running pytest..." | |
| pytest tests/ -x -q | |
| #!/bin/sh | |
| # Pre-commit hook for reddit-cli | |
| set -e | |
| # Run ruff linting | |
| echo "Running ruff lint..." | |
| ruff check reddit_cli tests | |
| # Run mypy type checking | |
| echo "Running mypy type check..." | |
| mypy reddit_cli | |
| # Run pytest | |
| echo "Running pytest..." | |
| pytest tests/ -x -q |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.husky/pre-commit around lines 1 - 14, Add "set -e" (and optionally "set -o
pipefail") immediately after the shebang in the pre-commit shell script so the
script fails fast on any ruff, mypy, or pytest error; update the top of the
script (the line after "# Pre-commit hook for reddit-cli") to include these
options. Also consider removing the duplicated checks from
.pre-commit-config.yaml or the husky script and centralize hooks in one
mechanism (either husky or pre-commit) to avoid maintenance duplication.
| Run this command to enable shell completion for your shell: | ||
| - bash: reddit completion bash >> ~/.bashrc | ||
| - zsh: reddit completion zsh >> ~/.zshrc | ||
| - fish: reddit completion fish > ~/.config/fish/completions/reddit.fish | ||
| - powershell: reddit completion powershell >> $PROFILE |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and check its size
fd -e py -path "*reddit_cli/__init__.py" | head -5Repository: AliiiBenn/reddit-cli
Length of output: 232
🏁 Script executed:
# Find and read the file
find . -type f -name "__init__.py" -path "*reddit_cli*" | head -1 | xargs wc -lRepository: AliiiBenn/reddit-cli
Length of output: 92
🏁 Script executed:
# Read the specific sections of interest
find . -type f -name "__init__.py" -path "*reddit_cli*" -exec sed -n '115,135p' {} +Repository: AliiiBenn/reddit-cli
Length of output: 823
Update the documentation examples to use the --shell option.
The examples at lines 125-129 show reddit completion bash, but the shell parameter is defined as a typer.Option with --shell (line 121). Following the current examples will error; they must use the correct syntax with output redirections:
- bash:
reddit completion --shell bash >> ~/.bashrc - zsh:
reddit completion --shell zsh >> ~/.zshrc - fish:
reddit completion --shell fish > ~/.config/fish/completions/reddit.fish - powershell:
reddit completion --shell powershell >> $PROFILE
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/__init__.py` around lines 125 - 129, The usage examples for the
completion command are incorrect: the CLI defines the shell parameter as a Typer
option named --shell (see the typer.Option for shell around the completion
command), so update the four example lines to call the command with the flag
(e.g. "reddit completion --shell bash >> ~/.bashrc", "reddit completion --shell
zsh >> ~/.zshrc", "reddit completion --shell fish >
~/.config/fish/completions/reddit.fish", "reddit completion --shell powershell
>> $PROFILE") so they match the completion function's --shell parameter and
include the proper redirections.
| # Valid Values Constants | ||
| # ============================================================================= | ||
|
|
||
| VALID_FORMAT_VALUES = ["display", "sql", "csv", "xlsx"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how commands validate format and if they use custom valid_values
echo "=== Checking format validation in commands ==="
rg -n "_validate_format" --type=py -A2Repository: AliiiBenn/reddit-cli
Length of output: 3300
🏁 Script executed:
# Read the _validate_format function to see what it does when valid_values is None
cat -n reddit_cli/commands/_shared.py | sed -n '58,71p'Repository: AliiiBenn/reddit-cli
Length of output: 678
🏁 Script executed:
# Check if JSON export is supported in the export module
find . -type f -name "*.py" -path "*/export*" | head -20Repository: AliiiBenn/reddit-cli
Length of output: 86
🏁 Script executed:
# Look for any JSON export functionality
rg -n "json" --type=py -i | grep -i "export\|format" | head -20Repository: AliiiBenn/reddit-cli
Length of output: 1598
🏁 Script executed:
# Check if other command modules define their own VALID_FORMAT_VALUES
rg -n "VALID_FORMAT_VALUES" --type=pyRepository: AliiiBenn/reddit-cli
Length of output: 1595
🏁 Script executed:
# Check the full comments.py to see if it defines its own VALID_FORMAT_VALUES
cat -n reddit_cli/commands/comments.py | head -30Repository: AliiiBenn/reddit-cli
Length of output: 1020
🏁 Script executed:
# Check which modules import VALID_FORMAT_VALUES from _shared
rg -n "from.*_shared.*import" --type=py -A2Repository: AliiiBenn/reddit-cli
Length of output: 1096
🏁 Script executed:
# Check what export functions subreddit.py imports
head -30 reddit_cli/commands/subreddit.pyRepository: AliiiBenn/reddit-cli
Length of output: 948
🏁 Script executed:
# Check if there are any JSON export calls in subreddit.py
rg -n "to_json" reddit_cli/commands/subreddit.pyRepository: AliiiBenn/reddit-cli
Length of output: 46
🏁 Script executed:
# Check what export functions comments.py imports and uses
grep -n "to_json\|comments_to_json" reddit_cli/commands/comments.pyRepository: AliiiBenn/reddit-cli
Length of output: 140
🏁 Script executed:
# Check how comments.py calls _validate_format
rg -n "_validate_format" reddit_cli/commands/comments.py -B2 -A2Repository: AliiiBenn/reddit-cli
Length of output: 479
🏁 Script executed:
# Check if search.py and browse.py also have this issue
rg -n "_validate_format" reddit_cli/commands/search.py -B1 -A1Repository: AliiiBenn/reddit-cli
Length of output: 217
🏁 Script executed:
# Check browse.py too
rg -n "_validate_format" reddit_cli/commands/browse.py -B1 -A1Repository: AliiiBenn/reddit-cli
Length of output: 217
Add "json" to VALID_FORMAT_VALUES in _shared.py and fix validation bug in comments.py.
JSON export is supported (export.py has comments_to_json(), posts_to_json(), etc.), but "json" is missing from the default VALID_FORMAT_VALUES. This breaks format validation in comments.py, which defines its own VALID_FORMAT_VALUES with "json" but fails to pass it to _validate_format() at lines 139 and 174. As a result, users cannot use --format json despite JSON export code existing in the module.
- Add "json" to
VALID_FORMAT_VALUESin _shared.py - Fix comments.py to pass
VALID_FORMAT_VALUESto_validate_format()calls
Proposed fixes
_shared.py line 13:
-VALID_FORMAT_VALUES = ["display", "sql", "csv", "xlsx"]
+VALID_FORMAT_VALUES = ["display", "sql", "csv", "xlsx", "json"]comments.py lines 139 and 174:
- _validate_format(format)
+ _validate_format(format, VALID_FORMAT_VALUES)📝 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.
| VALID_FORMAT_VALUES = ["display", "sql", "csv", "xlsx"] | |
| VALID_FORMAT_VALUES = ["display", "sql", "csv", "xlsx", "json"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/commands/_shared.py` at line 13, Add "json" to the global
VALID_FORMAT_VALUES list in _shared.py and update the two calls to
_validate_format() in comments.py to pass the module's VALID_FORMAT_VALUES so
JSON becomes an accepted format; specifically, edit VALID_FORMAT_VALUES =
["display", "sql", "csv", "xlsx"] in _shared.py to include "json" and change the
_validate_format(...) invocations in comments.py (the calls around where format
is validated) to supply the comments.py VALID_FORMAT_VALUES constant as the
second argument to _validate_format().
| """Display posts in terminal format using Rich.""" | ||
| print_posts(posts) |
There was a problem hiding this comment.
Restore post IDs in the Rich list view.
reddit_cli.ui.print_posts() only renders score/title/subreddit/author/comments, so this swap drops the post ID from frontpage/home/best output. That removes the stable identifier users need to chain into reddit post or reddit comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/commands/navigation.py` around lines 88 - 89, The Rich list view
lost post IDs because the navigation command now calls print_posts(posts) which
only renders score/title/subreddit/author/comments; update the rendering to
include the post ID so users can reference it for commands like `reddit post` or
`reddit comments`. Modify reddit_cli.ui.print_posts (or the caller in
reddit_cli.commands.navigation where print_posts is invoked) to include the
post.id/ID in the displayed row (e.g., prepend or add an ID column) and ensure
the ID value is passed through from the posts data structure so the
frontpage/home/best output contains the stable identifier.
| def setup_logging(level: int = INFO) -> None: | ||
| """Configure logging for the application. | ||
|
|
||
| Args: | ||
| level: Logging level (DEBUG, INFO, WARNING, ERROR). Default is INFO. | ||
| """ | ||
| # Configure root logger | ||
| logging.basicConfig( | ||
| level=level, | ||
| format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", | ||
| datefmt="%Y-%m-%d %H:%M:%S", | ||
| handlers=[ | ||
| logging.StreamHandler(sys.stderr) | ||
| ] | ||
| ) | ||
|
|
||
| # Ensure our logger uses the configured level | ||
| logger.setLevel(level) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if setup_logging is called anywhere in the codebase
echo "=== Searching for setup_logging calls ==="
rg -n "setup_logging\s*\(" --type=py
echo ""
echo "=== Checking CLI entry point for logging setup ==="
# Find the main app definition
rg -n -A5 "app\s*=\s*typer" --type=pyRepository: AliiiBenn/reddit-cli
Length of output: 888
🏁 Script executed:
#!/bin/bash
# Check if setup_logging is imported anywhere
echo "=== Searching for setup_logging imports ==="
rg -n "from.*setup_logging|import.*setup_logging" --type=py
echo ""
echo "=== Checking for __main__ blocks ==="
rg -n "if __name__.*==.*__main__" --type=py
echo ""
echo "=== Checking get_logger usage in errors.py ==="
rg -n "get_logger\|setup_logging" reddit_cli/errors.py --type=py
echo ""
echo "=== Checking the __init__.py file entry point ==="
head -20 reddit_cli/__init__.pyRepository: AliiiBenn/reddit-cli
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Read the __main__ block
echo "=== __main__ block in __init__.py ==="
sed -n '163,180p' reddit_cli/__init__.py
echo ""
echo "=== Check if errors.py exists and search for logger usage ==="
ls -la reddit_cli/errors.py 2>/dev/null && echo "File exists"
rg -n "logger|get_logger" reddit_cli/errors.py --type=py
echo ""
echo "=== Search get_logger usage across entire codebase ==="
rg -n "get_logger" --type=py
echo ""
echo "=== Check logging.py to see full module structure ==="
cat reddit_cli/logging.pyRepository: AliiiBenn/reddit-cli
Length of output: 2273
setup_logging() is never called at CLI startup.
The setup_logging() function is defined but never imported or invoked anywhere in the codebase. The __main__ block in reddit_cli/__init__.py only calls app() without initializing logging. As a result, get_logger() (used in reddit_cli/errors.py) returns a logger that uses Python's default configuration (WARNING level, basic format) instead of the configured timestamped format from basicConfig().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/logging.py` around lines 15 - 32, The CLI never initializes the
logging config because setup_logging() in logging.py is not invoked at startup;
update the CLI entrypoint (the __main__ block that calls app() in
reddit_cli/__init__.py) to import and call setup_logging() (with the desired
level) before calling app() so get_logger() in reddit_cli/errors.py and
elsewhere will use the configured basicConfig (timestamped format and provided
handlers) rather than Python's default logging; ensure you reference the
setup_logging function and call it prior to any logger usage or app()
invocation.
| # Console instance for stderr output (user-facing messages) | ||
| # Use a wide terminal width to prevent text wrapping | ||
| console = Console(stderr=True, width=120) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ui.py" -path "*/reddit_cli/*" -type fRepository: AliiiBenn/reddit-cli
Length of output: 82
🏁 Script executed:
cat -n reddit_cli/ui.py | head -150Repository: AliiiBenn/reddit-cli
Length of output: 4136
🏁 Script executed:
rg "console\." reddit_cli/ui.py -A 2Repository: AliiiBenn/reddit-cli
Length of output: 517
🏁 Script executed:
rg "print_progress" --type pyRepository: AliiiBenn/reddit-cli
Length of output: 126
🏁 Script executed:
rg "from.*ui import\|from.*\.ui import" --type pyRepository: AliiiBenn/reddit-cli
Length of output: 46
🏁 Script executed:
rg "console" reddit_cli/ --type py -B 2 -A 2Repository: AliiiBenn/reddit-cli
Length of output: 1933
Route display output to stdout, keep progress/status on stderr.
The console instance with stderr=True is used for tables (print_table, print_posts) and comments (print_comments), which are normal CLI data output that belongs on stdout. With the current setup, users cannot pipe or redirect this output as expected (e.g., reddit-cli posts | grep title won't capture the data). Rich's stderr=True is documented for error and status consoles, not display output.
Create a separate console for normal output on stdout and move progress reporting to a dedicated stderr console:
Suggested split
-# Console instance for stderr output (user-facing messages)
-# Use a wide terminal width to prevent text wrapping
-console = Console(stderr=True, width=120)
+# Console for normal CLI output
+console = Console(width=120)
+
+# Separate console for progress / status / error-oriented output
+status_console = Console(stderr=True, width=120)Then update print_progress at lines 110–113:
def print_progress(message: str) -> Progress:
return Progress(
SpinnerColumn(),
TextColumn("[progress.description]{task.description}"),
- console=console,
+ console=status_console,
)📝 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.
| # Console instance for stderr output (user-facing messages) | |
| # Use a wide terminal width to prevent text wrapping | |
| console = Console(stderr=True, width=120) | |
| # Console for normal CLI output | |
| console = Console(width=120) | |
| # Separate console for progress / status / error-oriented output | |
| status_console = Console(stderr=True, width=120) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/ui.py` around lines 8 - 10, The current shared Console named
console is created with stderr=True and is used for normal CLI output
(print_table, print_posts, print_comments) which should go to stdout; create two
consoles instead (e.g., out_console = Console(stderr=False, width=120) for
normal/tabular output and err_console = Console(stderr=True, width=120) for
progress/status), replace usages of console in print_table, print_posts,
print_comments to use out_console, and update print_progress to write to
err_console (and any other status/logging helpers to use err_console) so
piping/redirecting works as expected.
| for row in data: | ||
| table.add_row(*[str(row.get(col, "")) for col in columns]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the repository structure
git ls-files | head -20Repository: AliiiBenn/reddit-cli
Length of output: 936
🏁 Script executed:
#!/bin/bash
# Locate and examine the reddit_cli/ui.py file
fd "ui.py" --type fRepository: AliiiBenn/reddit-cli
Length of output: 80
🏁 Script executed:
#!/bin/bash
# Check if reddit_cli/ui.py exists and get its size
test -f reddit_cli/ui.py && wc -l reddit_cli/ui.pyRepository: AliiiBenn/reddit-cli
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Read the ui.py file to examine the code
cat -n reddit_cli/ui.pyRepository: AliiiBenn/reddit-cli
Length of output: 4136
🌐 Web query:
Rich library Table markup rendering behavior cells
💡 Result:
In Rich (Python), a Table cell can contain any renderable, including a plain str (via Table.add_row(...)). When a cell is a string, Rich will render it the same way it renders any printed string: as Console Markup (bbcode-like tags) as long as the Console has markup enabled. [1], [2], [3]
So:
table.add_row("[bold red]Error[/]")will render “Error” in bold red (defaultConsole(markup=True)). [2], [3]- If you don’t want markup interpreted in cells:
- create the console with
Console(markup=False), or - escape
[as\[(Console Markup escaping), or - pass a
Text(...)object instead of a string (so you control styling explicitly). [2], [1]
- create the console with
If you do want to force markup parsing into a Text object (e.g., to pre-parse markup), use Text.from_markup(...). [4]
Sources
[1] https://rich.readthedocs.io/en/latest/reference/table.html
[2] https://rich.readthedocs.io/en/latest/console.html
[3] https://rich.readthedocs.io/en/latest/protocol.html
[4] https://deepwiki.com/Textualize/rich/3-renderables
🏁 Script executed:
#!/bin/bash
# Let's check Rich library documentation and verify the escape function exists
python3 << 'EOF'
try:
from rich.markup import escape
print("rich.markup.escape exists: True")
print(f"Function signature: {escape.__doc__}")
# Test the escape function
test_str = "[bold red]Hello[/bold red]"
escaped = escape(test_str)
print(f"\nTest escape:")
print(f" Input: {test_str}")
print(f" Output: {escaped}")
except ImportError as e:
print(f"ImportError: {e}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: AliiiBenn/reddit-cli
Length of output: 399
Escape untrusted Reddit data before passing to Rich's Table and console.
Rich renders plain strings as Console Markup by default, interpreting [...] syntax as bbcode-like formatting tags. User-generated content from the Reddit API (titles, subreddit names, authors, comment bodies) can contain these characters, causing unexpected rendering or raising MarkupError exceptions.
Use rich.markup.escape() to sanitize user content before passing it to table.add_row() or console.print():
Affected locations and fixes
+from rich.markup import escape
...
- table.add_row(*[str(row.get(col, "")) for col in columns])
+ table.add_row(*[escape(str(row.get(col, ""))) for col in columns])
...
table.add_row(
score_str,
- post.title,
- f"r/{post.subreddit}",
- post.author,
+ escape(str(post.title)),
+ escape(f"r/{post.subreddit}"),
+ escape(str(post.author)),
str(post.num_comments),
)
...
console.print(
- f"{prefix}[yellow]{comment.score:>4}[/yellow] [bold green]{comment.author}[/bold green]"
+ f"{prefix}[yellow]{comment.score:>4}[/yellow] [bold green]{escape(str(comment.author))}[/bold green]"
)
...
- console.print(f"{prefix}[dim]{body}[/dim]")
+ console.print(f"{prefix}[dim]{escape(body)}[/dim]")Applies to lines 29-30, 61-67, and 84-93.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/ui.py` around lines 29 - 30, Sanitize all user-provided Reddit
strings before passing them to Rich to avoid markup interpretation: wrap values
passed into table.add_row (the loop that builds rows using columns and data) and
any console.print calls that render titles, subreddit names, authors, or comment
bodies with rich.markup.escape() so you pass escaped strings instead of raw
values; update the data->row creation (the list comprehension feeding
table.add_row) and the blocks that call console.print(...) to call
rich.markup.escape on each user-originated string (e.g., title, author,
subreddit, body) before formatting/printing.
| async def test_reddit_client_context_manager(): | ||
| """Test that RedditClient properly initializes and cleans up via context manager.""" | ||
| async with RedditClient() as client: | ||
| # Test that client is properly initialized | ||
| assert client._client is not None | ||
| # Test that we can make a basic request (will be mocked) | ||
| assert client.BASE_URL == "https://www.reddit.com" | ||
| assert client.TIMEOUT == 10.0 | ||
| assert client.USER_AGENT == "better-reddit-cli/0.4.4" | ||
| assert client.MAX_RETRIES == 3 | ||
| assert client.INITIAL_BACKOFF == 1.0 | ||
|
|
||
| # After context exit, client should be closed | ||
| # Note: We cannot directly test _client is None because the client | ||
| # is set to None in __aexit__ but the object persists |
There was a problem hiding this comment.
These lifecycle tests don’t exercise teardown or same-instance re-entry.
client is still in scope after the first async with, so you can assert cleanup on exit there. The second test constructs a fresh RedditClient() on every iteration, which doesn’t catch state leakage across re-entering the same instance.
Suggested coverage change
`@pytest.mark.asyncio`
async def test_reddit_client_context_manager():
"""Test that RedditClient properly initializes and cleans up via context manager."""
- async with RedditClient() as client:
+ client = RedditClient()
+ async with client:
# Test that client is properly initialized
assert client._client is not None
# Test that we can make a basic request (will be mocked)
assert client.BASE_URL == "https://www.reddit.com"
assert client.TIMEOUT == 10.0
assert client.USER_AGENT == "better-reddit-cli/0.4.4"
assert client.MAX_RETRIES == 3
assert client.INITIAL_BACKOFF == 1.0
- # After context exit, client should be closed
- # Note: We cannot directly test _client is None because the client
- # is set to None in __aexit__ but the object persists
+ assert client._client is None
...
`@pytest.mark.asyncio`
async def test_reddit_client_context_manager_multiple_entries():
"""Test that RedditClient can be used in multiple context manager entries."""
- for _ in range(3):
- async with RedditClient() as client:
+ client = RedditClient()
+ for _ in range(3):
+ async with client:
assert client._client is not None
+ assert client._client is NoneAlso applies to: 28-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_async.py` around lines 10 - 24, The test currently doesn't verify
teardown or re-entry on the same RedditClient instance; update
test_reddit_client_context_manager to reuse the same RedditClient object: after
the first async with RedditClient() as client block assert that client._client
is None to confirm __aexit__ cleaned up, then perform a second `async with
client as client2` (re-enter the same instance) to ensure __aenter__
reinitializes the client and then again assert client._client is None after
exit; reference the RedditClient class and its __aenter__/__aexit__ behavior and
the _client attribute to locate where to change the test.
- Remove unused imports (os, pytest, CliRunner) - Fix bare except clauses in xlsx_export.py - Remove duplicate TestNavigationDisplayFormat class - Rename duplicate TestPost class to TestPostFormat - Add types-openpyxl to dev dependencies - Add mypy ignore_missing_imports config - Fix f-string issues in __init__.py completion command Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The openpyxl Workbook.active property can return None according to type stubs, but in practice it never does when using a standard Workbook. Added explicit type ignores to satisfy mypy while keeping the code clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@reddit_cli/cache.py`:
- Around line 62-85: get_cached and set_cached in reddit_cli/cache.py are
unused; integrate them into the HTTP flow by updating RedditClient.get to
consult get_cached(endpoint, params) before making a network request and
returning cached data when present, and after a successful fetch call
set_cached(endpoint, params, response) to persist responses; ensure callers in
posts.py and subreddits.py keep calling RedditClient.get unchanged so they
automatically benefit from caching, and handle cache miss/JSON decode failures
by falling back to the existing network path.
- Around line 49-58: The except clauses that currently catch
(json.JSONDecodeError, KeyError, ValueError) when reading/parsing cache files
should also include OSError so filesystem errors (permissions, race deletes) are
treated as cache misses; update the exception tuples in the cache-reading
routines (the function that opens/parses the cache and the other cache-read
except block referenced in the diff) to catch (json.JSONDecodeError, KeyError,
ValueError, OSError), keeping existing behavior consistent with set_cached and
clear_cache.
- Around line 31-33: _get_cache_file_path currently calls CACHE_DIR.mkdir
(mutating FS) and get_cached/_is_cache_valid perform file ops without OSError
handling; move the CACHE_DIR.mkdir(parents=True, exist_ok=True) call out of
_get_cache_file_path and into set_cached so directory creation happens only on
writes, and add try/except OSError handling in get_cached and _is_cache_valid
(mirroring set_cached's behavior) to catch/handle filesystem errors when opening
or stat-ing cache files; update references to these functions (get_cached,
_get_cache_file_path, _is_cache_valid, set_cached) accordingly.
🪄 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: f9f65216-a283-419b-a3d8-ef6edc4dc66b
📒 Files selected for processing (9)
pyproject.tomlreddit_cli/__init__.pyreddit_cli/cache.pyreddit_cli/xlsx_export.pytests/test_cli_browse.pytests/test_cli_navigation.pytests/test_cli_post.pytests/test_cli_subreddit.pytests/test_export.py
💤 Files with no reviewable changes (3)
- tests/test_cli_browse.py
- tests/test_export.py
- tests/test_cli_subreddit.py
✅ Files skipped from review due to trivial changes (3)
- tests/test_cli_post.py
- reddit_cli/xlsx_export.py
- tests/test_cli_navigation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- reddit_cli/init.py
| # Create cache directory if it doesn't exist | ||
| CACHE_DIR.mkdir(parents=True, exist_ok=True) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n reddit_cli/cache.pyRepository: AliiiBenn/reddit-cli
Length of output: 4357
🏁 Script executed:
rg "get_cached|set_cached" --type py -A 2 -B 2Repository: AliiiBenn/reddit-cli
Length of output: 505
🏁 Script executed:
rg "from.*cache import|import.*cache" --type pyRepository: AliiiBenn/reddit-cli
Length of output: 46
🏁 Script executed:
rg "get_cached|set_cached" --type pyRepository: AliiiBenn/reddit-cli
Length of output: 253
🏁 Script executed:
fd -t f -e py . -x grep -l "RedditClient\|get\|post" {} \; | head -5Repository: AliiiBenn/reddit-cli
Length of output: 186
🏁 Script executed:
fd -t f -name "*.py" reddit_cli/ | grep -v __pycache__Repository: AliiiBenn/reddit-cli
Length of output: 294
🏁 Script executed:
fd -t f "\.py$" reddit_cli/Repository: AliiiBenn/reddit-cli
Length of output: 604
🏁 Script executed:
cat reddit_cli/reddit/base.py | head -80Repository: AliiiBenn/reddit-cli
Length of output: 2744
Move cache-directory creation out of the read path.
Line 32 makes get_cached() mutate the filesystem (and potentially fail) just to compute a key. This should happen only on write (set_cached), not in _get_cache_file_path. Additionally, get_cached() and _is_cache_valid() lack OSError handling for file operations, while set_cached already has it.
Proposed fix
def _get_cache_file_path(endpoint: str, params: dict | None = None) -> Path:
@@
- # Create cache directory if it doesn't exist
- CACHE_DIR.mkdir(parents=True, exist_ok=True)
-
return CACHE_DIR / f"{key_hash}.json"
@@
def set_cached(endpoint: str, params: dict | None, response: dict) -> None:
@@
+ try:
+ CACHE_DIR.mkdir(parents=True, exist_ok=True)
+ except OSError:
+ return
+ cache_file = _get_cache_file_path(endpoint, params)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/cache.py` around lines 31 - 33, _get_cache_file_path currently
calls CACHE_DIR.mkdir (mutating FS) and get_cached/_is_cache_valid perform file
ops without OSError handling; move the CACHE_DIR.mkdir(parents=True,
exist_ok=True) call out of _get_cache_file_path and into set_cached so directory
creation happens only on writes, and add try/except OSError handling in
get_cached and _is_cache_valid (mirroring set_cached's behavior) to catch/handle
filesystem errors when opening or stat-ing cache files; update references to
these functions (get_cached, _get_cache_file_path, _is_cache_valid, set_cached)
accordingly.
| try: | ||
| with open(cache_file, "r") as f: | ||
| cache_data = json.load(f) | ||
|
|
||
| cached_time = datetime.fromisoformat(cache_data["timestamp"]) | ||
| expiry_time = cached_time + timedelta(minutes=CACHE_TTL_MINUTES) | ||
|
|
||
| return datetime.now() < expiry_time | ||
| except (json.JSONDecodeError, KeyError, ValueError): | ||
| # Cache is invalid if we can't read or parse it |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the cache.py file
find . -name "cache.py" -path "*/reddit_cli/*" | head -5Repository: AliiiBenn/reddit-cli
Length of output: 85
🏁 Script executed:
# Read the file to see the actual code structure
cat -n reddit_cli/cache.pyRepository: AliiiBenn/reddit-cli
Length of output: 4357
Add OSError to exception handlers for filesystem read errors.
Line 50 and line 78 can raise OSError due to permissions, race conditions, or deleted files. These exceptions are currently uncaught and will break normal CLI flows instead of treating them as cache misses.
Add OSError to both except clauses:
Fix
except (json.JSONDecodeError, KeyError, ValueError, OSError):
# Cache is invalid if we can't read or parse it
return False except (json.JSONDecodeError, KeyError, OSError):
return NoneThis aligns with the existing OSError handling in set_cached() (line 105) and clear_cache() (line 116).
📝 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.
| try: | |
| with open(cache_file, "r") as f: | |
| cache_data = json.load(f) | |
| cached_time = datetime.fromisoformat(cache_data["timestamp"]) | |
| expiry_time = cached_time + timedelta(minutes=CACHE_TTL_MINUTES) | |
| return datetime.now() < expiry_time | |
| except (json.JSONDecodeError, KeyError, ValueError): | |
| # Cache is invalid if we can't read or parse it | |
| try: | |
| with open(cache_file, "r") as f: | |
| cache_data = json.load(f) | |
| cached_time = datetime.fromisoformat(cache_data["timestamp"]) | |
| expiry_time = cached_time + timedelta(minutes=CACHE_TTL_MINUTES) | |
| return datetime.now() < expiry_time | |
| except (json.JSONDecodeError, KeyError, ValueError, OSError): | |
| # Cache is invalid if we can't read or parse it |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/cache.py` around lines 49 - 58, The except clauses that currently
catch (json.JSONDecodeError, KeyError, ValueError) when reading/parsing cache
files should also include OSError so filesystem errors (permissions, race
deletes) are treated as cache misses; update the exception tuples in the
cache-reading routines (the function that opens/parses the cache and the other
cache-read except block referenced in the diff) to catch (json.JSONDecodeError,
KeyError, ValueError, OSError), keeping existing behavior consistent with
set_cached and clear_cache.
| def get_cached(endpoint: str, params: dict | None = None) -> dict | None: | ||
| """Get cached response if available and valid. | ||
|
|
||
| Args: | ||
| endpoint: API endpoint path | ||
| params: Query parameters dict | ||
|
|
||
| Returns: | ||
| Cached response dict if valid, None otherwise | ||
| """ | ||
| cache_file = _get_cache_file_path(endpoint, params) | ||
|
|
||
| if not _is_cache_valid(cache_file): | ||
| return None | ||
|
|
||
| try: | ||
| with open(cache_file, "r") as f: | ||
| cache_data = json.load(f) | ||
| return cache_data.get("response") | ||
| except (json.JSONDecodeError, KeyError): | ||
| return None | ||
|
|
||
|
|
||
| def set_cached(endpoint: str, params: dict | None, response: dict) -> None: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether cache helpers are wired into runtime request flows.
rg -nP --type=py -C2 '\bget_cached\s*\('
rg -nP --type=py -C2 '\bset_cached\s*\('
rg -nP --type=py -C2 '\bclass\s+RedditClient\b|\bdef\s+get\s*\('Repository: AliiiBenn/reddit-cli
Length of output: 1355
🏁 Script executed:
# Search for actual calls to cache functions (not definitions)
rg -n 'get_cached\(' reddit_cli/ --type=py | grep -v 'def get_cached'
rg -n 'set_cached\(' reddit_cli/ --type=py | grep -v 'def set_cached'Repository: AliiiBenn/reddit-cli
Length of output: 46
🏁 Script executed:
# Check the full implementation of RedditClient.get() method
sed -n '29,60p' reddit_cli/reddit/base.pyRepository: AliiiBenn/reddit-cli
Length of output: 1232
🏁 Script executed:
# Check the implementation of request methods in posts.py and subreddits.py
head -100 reddit_cli/reddit/posts.py
head -100 reddit_cli/reddit/subreddits.pyRepository: AliiiBenn/reddit-cli
Length of output: 6670
Cache helpers are not integrated into the request flow.
get_cached() and set_cached() are defined in reddit_cli/cache.py but have no call sites anywhere in the codebase. RedditClient.get() makes direct HTTP requests without consulting the cache, and all request methods in posts.py and subreddits.py invoke self._client.get() directly. The cache module is currently unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@reddit_cli/cache.py` around lines 62 - 85, get_cached and set_cached in
reddit_cli/cache.py are unused; integrate them into the HTTP flow by updating
RedditClient.get to consult get_cached(endpoint, params) before making a network
request and returning cached data when present, and after a successful fetch
call set_cached(endpoint, params, response) to persist responses; ensure callers
in posts.py and subreddits.py keep calling RedditClient.get unchanged so they
automatically benefit from caching, and handle cache miss/JSON decode failures
by falling back to the existing network path.
1 participant
Summary
reddit_cli/commands/_shared.py--outputTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests