Conversation
📝 WalkthroughWalkthroughVersion strings bumped to v1.8.9 across build/packaging and docs; pipeline job variables renamed; TCG set target switched to me02.5 with expanded variant filtering and product mapping; added tests for JSON and secret retrievers; minor UI and whitespace edits. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@card_data/pipelines/tests/json_retriever_test.py`:
- Around line 113-126: The test test_fetch_json_custom_timeout currently only
checks the response body and doesn't verify the timeout argument is forwarded to
fetch_json; change the test to patch requests.get (e.g., with
unittest.mock.patch or monkeypatch) to capture its call kwargs, call
fetch_json("https://api.example.com/data", timeout=5), and assert that the
patched requests.get was invoked with timeout=5; reference fetch_json and
test_fetch_json_custom_timeout to locate where to add the patch and the
assertion.
🧹 Nitpick comments (3)
card_data/pipelines/tests/secret_retriever_test.py (2)
1-4:sys.pathmanipulation for imports.Consider configuring the test runner (e.g., via
pyproject.tomlor aconftest.pywithsys.pathadjustment) so individual test files don't need this boilerplate. Not blocking, just a maintainability note.
14-14: Prefix unusedmock_get_sessionwith_to silence Ruff ARG001.The
@patchforbotocore.session.get_sessionis necessary to prevent real AWS calls, but the injected mock is never referenced. Prefixing the parameter with_across all ten tests communicates intent and satisfies the linter.Proposed fix (apply to all test functions)
-def test_fetch_secret_success(mock_get_session, mock_secret_cache_cls): +def test_fetch_secret_success(_mock_get_session, mock_secret_cache_cls):card_data/pipelines/tests/json_retriever_test.py (1)
1-4: Samesys.pathmanipulation note as the sibling test file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 33-34: Add a blank line between the image markdown
""
and the horizontal rule marker "---" so the three dashes are parsed as a
thematic break instead of a setext-style heading (update the README.md where the
image link and the '---' occur).
🧹 Nitpick comments (2)
card_data/pipelines/tests/json_retriever_test.py (1)
1-4: Consider using aconftest.pyor packaging setup instead ofsys.pathmanipulation.Inserting into
sys.pathat runtime is fragile and can mask import issues. Aconftest.pyat thecard_datalevel or an editable install (pip install -e .) would make imports work cleanly without path hacking..github/workflows/python_lint.yml (1)
31-32: Consider pinning the Ruff version for reproducible lint results.
uv tool install ruffinstalls the latest version, meaning a new Ruff release could introduce new lint rules or change behavior and break this workflow unexpectedly. Pinning (e.g.,uv tool install ruff==0.9.x) gives you control over when to upgrade.
1 participant
Summary by CodeRabbit
Chores
Documentation
Improvements
Tests