Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds short-term typing notes and widespread type-hint improvements across the test suite and configuration: tests now annotate fixtures (SqliterDB, MockerFixture, Path, pytest.LogCaptureFixture, etc.) and use TYPE_CHECKING casts; pyproject.toml had mypy overrides removed and ANN001 removed from per-file ruff ignores. No behavioural changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
seapagan
changed the title
seapagan
changed the title
![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
🧹 Nitpick comments (2)
pyproject.toml (1)
157-159: Consider deleting the commented-out[tool.mypy.overrides]block rather than keeping it.Commented-out TOML config has no effect and can mislead future readers into thinking the overrides are temporarily disabled. Since this PR permanently removes the suppression, the dead config is better removed entirely.
🗑️ Proposed change
-#[[tool.mypy.overrides]] -#disable_error_code = ["method-assign", "no-untyped-def", "attr-defined"] -#module = "tests.*" - [tool.pytest.ini_options]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 157 - 159, Remove the commented-out [tool.mypy.overrides] block from pyproject.toml (the three commented lines starting with #[[tool.mypy.overrides]] and the following disable_error_code/module lines) so the dead TOML config is deleted rather than left commented out; simply delete those three comment lines referencing the overrides to avoid misleading future readers.tests/test_orm_fields.py (1)
201-201: Minor: the"Union"branch in the set is unreachable given theskipifguard.The test is already gated on
sys.version_info >= (3, 10), sotype(pep604_union).__name__will always be"UnionType"inside this test. The"Union"entry is dead code. This has no impact on correctness, but if future readers see it they may wonder whether the test can actually produce atyping.Unionobject.🔍 Simplified assertion
- assert type(pep604_union).__name__ in {"UnionType", "Union"} + assert type(pep604_union).__name__ == "UnionType"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_orm_fields.py` at line 201, The assertion uses an unreachable branch: update the check on pep604_union to only expect "UnionType" (e.g., replace assert type(pep604_union).__name__ in {"UnionType", "Union"} with assert type(pep604_union).__name__ == "UnionType") so the test matches the sys.version_info >= (3, 10) guard; refer to the existing pep604_union variable and the assert expression to locate and change the assertion.
🤖 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/test_query.py`:
- Line 191: The method fetch_last_multiple_results should be renamed to include
the pytest discovery prefix — change its name to
test_fetch_last_multiple_results (update any internal references or calls to
this method if present) so pytest will discover and run it; ensure the function
signature and parameters (db_mock: SqliterDB) remain unchanged.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 157-159: Remove the commented-out [tool.mypy.overrides] block from
pyproject.toml (the three commented lines starting with #[[tool.mypy.overrides]]
and the following disable_error_code/module lines) so the dead TOML config is
deleted rather than left commented out; simply delete those three comment lines
referencing the overrides to avoid misleading future readers.
In `@tests/test_orm_fields.py`:
- Line 201: The assertion uses an unreachable branch: update the check on
pep604_union to only expect "UnionType" (e.g., replace assert
type(pep604_union).__name__ in {"UnionType", "Union"} with assert
type(pep604_union).__name__ == "UnionType") so the test matches the
sys.version_info >= (3, 10) guard; refer to the existing pep604_union variable
and the assert expression to locate and change the assertion.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
TODO.mdpyproject.tomltests/conftest.pytests/test_advanced_filters.pytests/test_aggregates.pytests/test_bulk_update.pytests/test_cache.pytests/test_context_manager.pytests/test_dates.pytests/test_debug_logging.pytests/test_drop_table.pytests/test_execeptions.pytests/test_foreign_keys.pytests/test_foreign_keys_orm.pytests/test_m2m.pytests/test_optional_fields.pytests/test_order_method.pytests/test_orm_fields.pytests/test_prefetch_related.pytests/test_properties.pytests/test_query.pytests/test_sqliter.pytests/test_timestamps.pytests/test_transaction_rollback.pytests/test_unique.pytests/tui/test_app.pytests/tui/test_demo_registry.pytests/tui/test_init.pytests/tui/test_main.pytests/tui/unit/test_demos_others.pytests/tui/widgets/test_demo_list.py
seapagan
changed the title
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
1 participant
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.
Summary
mypy --strictANN001Ruff ignore fortests/**/*.pyRationale
This brings strict typing standards to the test suite without reintroducing broad suppressions, and keeps runtime behavior unchanged. It also tightens lint policy by dropping the no-longer-needed
ANN001ignore for tests.Checks Run
poe ruff(passes)poe mypy(passes: no issues in 102 source files)poe test(passes: 1005 passed, 1 skipped)Summary by CodeRabbit
Refactor
Tests
Chores