Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds collection/deduplication of Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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 `@AGENTS.md`:
- Line 7: Change the phrase "single argument function" in AGENTS.md to the
hyphenated compound adjective "single-argument function"; locate the sentence
containing "Prefer keyword-only parameters (unless a very clear single argument
function): use `*` in Python signatures and destructured options objects in
TypeScript." and replace "single argument" with "single-argument" so the
compound adjective is correctly hyphenated.
- Around line 15-19: Edit the "### Python Testing" section in AGENTS.md: ensure
there is exactly one blank line immediately before the "### Python Testing"
heading (add a blank line above the heading if missing) and remove the extra
blank line after the paragraph (the stray blank line at the end of the
subsection). Keep the content unchanged, only adjust surrounding blank lines
around the "### Python Testing" heading and its paragraph.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6aaf601a-b442-433e-98c0-6ef9120a377e
📒 Files selected for processing (7)
.claude/helpers/merge-claude-settings.sh.coderabbit.yaml.devcontainer/devcontainer.json.devcontainer/on-create-command.shAGENTS.mdextensions/context.pytemplate/.devcontainer/on-create-command.sh.jinja-base
Comment on lines
+15
to
+19
| ### Python Testing | ||
| - When using `mocker.spy` on a class-level method (including inherited ones), the spy records the unbound call, so assertions need `ANY` as the first argument to match self: `spy.assert_called_once_with(ANY, expected_arg)` | ||
| - Before writing new mock/spy helpers, check the `tests/unit/` folder for pre-built helpers in files like `fixtures.py` or `*mocks.py` | ||
|
|
||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
New Python Testing subsection is helpful; minor formatting nit.
The mocker.spy guidance about ANY for the self argument is a valuable gotcha to document. The advice to check existing helpers prevents duplication.
There's an extra blank line at line 19, and per MD022, the heading at line 15 should have a blank line above it (line 14 is blank but line 13 ends content).
,
📝 Optional formatting fix
- Prefer using random values in tests rather than arbitrary ones (e.g. the faker library, uuids, random.randint) when possible.
### Python Testing
+
- When using `mocker.spy` on a class-level method (including inherited ones), the spy records the unbound call, so assertions need `ANY` as the first argument to match self: `spy.assert_called_once_with(ANY, expected_arg)`
- Before writing new mock/spy helpers, check the `tests/unit/` folder for pre-built helpers in files like `fixtures.py` or `*mocks.py`
-
-📝 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
| ### Python Testing | |
| - When using `mocker.spy` on a class-level method (including inherited ones), the spy records the unbound call, so assertions need `ANY` as the first argument to match self: `spy.assert_called_once_with(ANY, expected_arg)` | |
| - Before writing new mock/spy helpers, check the `tests/unit/` folder for pre-built helpers in files like `fixtures.py` or `*mocks.py` | |
| - Prefer using random values in tests rather than arbitrary ones (e.g. the faker library, uuids, random.randint) when possible. | |
| ### Python Testing | |
| - When using `mocker.spy` on a class-level method (including inherited ones), the spy records the unbound call, so assertions need `ANY` as the first argument to match self: `spy.assert_called_once_with(ANY, expected_arg)` | |
| - Before writing new mock/spy helpers, check the `tests/unit/` folder for pre-built helpers in files like `fixtures.py` or `*mocks.py` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 15 - 19, Edit the "### Python Testing" section in
AGENTS.md: ensure there is exactly one blank line immediately before the "###
Python Testing" heading (add a blank line above the heading if missing) and
remove the extra blank line after the paragraph (the stray blank line at the end
of the subsection). Keep the content unchanged, only adjust surrounding blank
lines around the "### Python Testing" heading and its paragraph.
zendern
previously approved these changes
Mar 5, 2026
This was referenced Mar 13, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this change necessary?
Fix the settings merger so it merges the "ask" things
Update some agent instructions based on empirical experience
What side effects does this change have?
N/A
How is this change tested?
Downstream repo
Other
use
ciinstead ofinstallto actually use what's in package-lock.jsonSummary by CodeRabbit
Chores
Documentation