Conversation
WalkthroughVersion bumped to 0.17.0; mypy config changed to use a string for python_version. Added optional boolean Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 100: The mypy/python target in pyproject.toml is set to python_version =
"3.9" which conflicts with the project's declared minimum Python support
(requires-python >=3.8,<4 and py38 targets in classifiers/black/isort); change
python_version from "3.9" to "3.8" so mypy's target matches requires-python and
the existing black target-version/ isort py_version entries, ensuring
typing/syntax stays compatible with Python 3.8.
In `@tests/test_types.py`:
- Around line 115-121: Combine the two tests into one parametrized pytest case:
create a single test (e.g., test_policy_include_tools) decorated with
`@pytest.mark.parametrize`("value,expected", [(None, False), (True, True)]) or
with explicit values [False, True] and instantiate EvaluationRequest accordingly
(for None use default ctor) and assert req.policy_include_tools == expected;
ensure pytest is imported and reference EvaluationRequest in the test body to
replace the two existing functions test_policy_include_tools_defaults_false and
test_policy_include_tools_can_be_set.
| def test_policy_include_tools_defaults_false(self): | ||
| req = EvaluationRequest(input="test") | ||
| assert req.policy_include_tools is False | ||
|
|
||
| def test_policy_include_tools_can_be_set(self): | ||
| req = EvaluationRequest(input="test", policy_include_tools=True) | ||
| assert req.policy_include_tools is True |
There was a problem hiding this comment.
Parametrize the new policy_include_tools tests.
The two standalone tests can be combined into a single parametrized case to follow test guidelines and reduce duplication.
♻️ Suggested refactor
- def test_policy_include_tools_defaults_false(self):
- req = EvaluationRequest(input="test")
- assert req.policy_include_tools is False
-
- def test_policy_include_tools_can_be_set(self):
- req = EvaluationRequest(input="test", policy_include_tools=True)
- assert req.policy_include_tools is True
+ `@pytest.mark.parametrize`(
+ "kwargs,expected",
+ [
+ ({}, False),
+ ({"policy_include_tools": True}, True),
+ ],
+ )
+ def test_policy_include_tools(self, kwargs, expected):
+ req = EvaluationRequest(input="test", **kwargs)
+ assert req.policy_include_tools is expectedAs per coding guidelines, "tests/**/*.py: Use parametrized pytest test cases in the tests/ directory for comprehensive test coverage".
📝 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.
| def test_policy_include_tools_defaults_false(self): | |
| req = EvaluationRequest(input="test") | |
| assert req.policy_include_tools is False | |
| def test_policy_include_tools_can_be_set(self): | |
| req = EvaluationRequest(input="test", policy_include_tools=True) | |
| assert req.policy_include_tools is True | |
| `@pytest.mark.parametrize`( | |
| "kwargs,expected", | |
| [ | |
| ({}, False), | |
| ({"policy_include_tools": True}, True), | |
| ], | |
| ) | |
| def test_policy_include_tools(self, kwargs, expected): | |
| req = EvaluationRequest(input="test", **kwargs) | |
| assert req.policy_include_tools is expected |
🤖 Prompt for AI Agents
In `@tests/test_types.py` around lines 115 - 121, Combine the two tests into one
parametrized pytest case: create a single test (e.g., test_policy_include_tools)
decorated with `@pytest.mark.parametrize`("value,expected", [(None, False), (True,
True)]) or with explicit values [False, True] and instantiate EvaluationRequest
accordingly (for None use default ctor) and assert req.policy_include_tools ==
expected; ensure pytest is imported and reference EvaluationRequest in the test
body to replace the two existing functions
test_policy_include_tools_defaults_false and
test_policy_include_tools_can_be_set.
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
LGTM
Just take a look at rabbit's comment about mypy version
2 participants
Description
Related Issue
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.make codestyle.Summary by CodeRabbit
New Features
Chores
Tests