Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved a unit test and a test fixture Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/models/test_model.py (1)
5-5: Optional: Consider adding WPS rules to Ruff'slint.externalconfig.The static analysis reports that
WPS347is not a Ruff rule code. This noqa comment is intended for flake8/wemake-python-styleguide, which is fine since the project uses both tools. To suppress these Ruff warnings across the codebase, you could addWPSrules to the[tool.ruff.lint]externalconfiguration option inpyproject.toml.This is purely cosmetic and doesn't affect functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/test_model.py` at line 5, The import line in tests/unit/models/test_model.py (importing BaseModel, ModelList, to_snake_case) uses a noqa comment for WPS347 which Ruff doesn't recognize; update the Ruff config instead of per-line noqa by adding the WPS rule prefix to the external list in pyproject.toml: under [tool.ruff.lint] add "WPS" (or the appropriate WPS rule codes) to lint.external so Ruff accepts WPS-style noqa comments across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/models/test_model.py`:
- Line 5: The import line in tests/unit/models/test_model.py (importing
BaseModel, ModelList, to_snake_case) uses a noqa comment for WPS347 which Ruff
doesn't recognize; update the Ruff config instead of per-line noqa by adding the
WPS rule prefix to the external list in pyproject.toml: under [tool.ruff.lint]
add "WPS" (or the appropriate WPS rule codes) to lint.external so Ruff accepts
WPS-style noqa comments across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3a1fc864-f5c9-410c-804c-c9754f975f10
📒 Files selected for processing (2)
tests/unit/models/resource/test_resource_custom_key.pytests/unit/models/test_model.py
💤 Files with no reviewable changes (1)
- tests/unit/models/resource/test_resource_custom_key.py
tests/unit/models/test_model.py
Outdated
|
|
||
| result = ChargeResourceMock.from_response(response) | ||
|
|
||
| assert result.id == "1" |
There was a problem hiding this comment.
Is it possible to use only one assert here?
expected_charge = ChargResouceMock(id=..., amount=...)
assert result == expected_charge
Otherwise tests are fragile a little bit if new properties are added
There was a problem hiding this comment.
This test it only tests if from_response gets the data correctly based on the _data_key if that has a value.
A quick search show that this seems to only be used in test. Orignially this was reused for collections but not anymore as collections use _collection_key now.
I'm happy to delete that code and test.
expected_charge = ChargResouceMock(id=..., amount=...)
assert result == expected_charge
that assertion will fail, as expected_charge would not have the Meta setup with the Response inside.
There was a problem hiding this comment.
Code and test deleted
albertsola
force-pushed
the
model_unit_test
branch
2 times, most recently
from
9d6a1ad to
5adcaf9
Compare
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 `@tests/unit/models/test_model.py`:
- Around line 236-241: In test_model_list_extend rename the single-letter
comprehension variable `m` to a descriptive name (e.g., `model` or `item`) to
satisfy WPS111; update the list comprehension in the assertion ([m.id for m in
ml] -> [model.id for model in ml]) inside the test_model_list_extend function
that uses ModelList so the test still asserts ["1","2","3"] but no longer uses a
one-letter variable.
- Line 5: The test import uses a flake8 WPS suppression "# noqa: WPS347" which
Ruff flags as RUF102; update lint config so both linters agree by adding the WPS
codes to Ruff’s external-ignore list or replace the suppression with a
Ruff-compatible one. Edit pyproject.toml to add "WPS347" (and other needed
"WPS*" codes) under tool.ruff.lint.external or remove the WPS-specific noqa on
the import of BaseModel, ModelList, to_snake_case and instead use a
Ruff-supported noqa token so the import line no longer triggers RUF102.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7d6186ed-b87a-475d-8bf6-90727f6c4a89
📒 Files selected for processing (3)
mpt_api_client/models/model.pytests/unit/models/resource/test_resource_custom_key.pytests/unit/models/test_model.py
💤 Files with no reviewable changes (1)
- tests/unit/models/resource/test_resource_custom_key.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
albertsola
force-pushed
the
model_unit_test
branch
from
5adcaf9 to
aabb371
Compare
tests/unit/models/test_model.py
Outdated
|
|
||
| ml.insert(1, {"id": "2"}) # act | ||
|
|
||
| assert len(ml) == 3 |
There was a problem hiding this comment.
I think here you can also squash number of assets
[ml_item.id for ml_item in ml] == ["1", "2", "3"]
tests/unit/models/test_model.py
Outdated
| def test_model_list_process_item_scalar(): | ||
| ml = ModelList(["a", "b", "c"]) # act | ||
|
|
||
| assert ml[0] == "a" |
There was a problem hiding this comment.
And here
assert ml == ["a", "b", "c"]
tests/unit/models/test_model.py
Outdated
| container = ScalarListContainerDummy(tags=["a", "b", "c"]) # act | ||
|
|
||
| assert isinstance(container.tags, ModelList) | ||
| assert container.tags[0] == "a" |
There was a problem hiding this comment.
and here
assert container.tags == ["a", "b", "c"]
819020d to
790a769
Compare
|
3 participants
Closes MPT-18701
_data_keyusage from tests (DummyModel no longer declares_data_key = None)