refactor(inspector): remove sqla private usages - #203
Conversation
📝 WalkthroughWalkthrough
ChangesPublic reverse-relationship resolution in SQLAlchemyInspector
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
Additional details and impacted files@@ Coverage Diff @@
## main #203 +/- ##
==========================================
- Coverage 93.26% 93.26% -0.01%
==========================================
Files 69 72 +3
Lines 6338 6486 +148
Branches 853 853
==========================================
+ Hits 5911 6049 +138
- Misses 285 296 +11
+ Partials 142 141 -1 ☔ View full report in Codecov by Harness. |
410e2e4 to
016128a
Compare
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/dto/inspectors/sqlalchemy.py`:
- Around line 529-532: The code comment in the required method indicates that
the column property branch (returning any not column.nullable for column in
model_field.property.columns) is never executed in production since the caller
only invokes required() for relationships. Evaluate whether this dead code
should be removed entirely to maintain code cleanliness, or if it should be
explicitly documented with a comment explaining why it is being retained for
future-proofing purposes. If keeping it, add clarity to the existing comment to
justify its presence despite being unreachable in the current codebase.
In `@tests/unit/test_inspector.py`:
- Around line 79-81: Add a sanity check assertion immediately after the
`_ALL_RELATIONSHIPS` list comprehension to verify the expected number of
relationships are present. This assertion should check that the length of
`_ALL_RELATIONSHIPS` equals the expected count (10) and fail with a descriptive
message if the count is incorrect, catching any accidental model removals or
broken relationship definitions that would otherwise cause silent test failures.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: be5c0f4c-ad50-4b6d-a1c7-d7dddb03d53b
📒 Files selected for processing (2)
src/strawchemy/dto/inspectors/sqlalchemy.pytests/unit/test_inspector.py
| # NOTE: reached only for column properties; the sole caller invokes | ||
| # required() exclusively for relationships, so this branch is | ||
| # currently unexercised in production. | ||
| return any(not column.nullable for column in model_field.property.columns) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The change from columns_to_assign to columns is semantically correct.
The columns attribute is the public API for accessing the underlying column collection. The comment accurately notes this branch is not exercised in production (only relationships call required()), which matches the downstream usage in types.py where self.inspector.required(field.model_field) is called within a field.is_relation context block.
However, the comment could lead to dead-code removal assumptions. If this branch is truly unreachable, consider whether it should remain for future-proofing or be removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/strawchemy/dto/inspectors/sqlalchemy.py` around lines 529 - 532, The code
comment in the required method indicates that the column property branch
(returning any not column.nullable for column in model_field.property.columns)
is never executed in production since the caller only invokes required() for
relationships. Evaluate whether this dead code should be removed entirely to
maintain code cleanliness, or if it should be explicitly documented with a
comment explaining why it is being retained for future-proofing purposes. If
keeping it, add clarity to the existing comment to justify its presence despite
being unreachable in the current codebase.
| _ALL_RELATIONSHIPS = [ | ||
| rel for model in (Department, Group, User, Tag, Author, Book) for rel in inspect(model).relationships | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider adding a sanity check for relationship count.
The _ALL_RELATIONSHIPS collection is built dynamically. If a model is accidentally removed or a relationship definition is broken, the test would silently run with fewer cases. Consider adding a brief assertion on the expected count.
_ALL_RELATIONSHIPS = [
rel for model in (Department, Group, User, Tag, Author, Book) for rel in inspect(model).relationships
]
# Sanity check: ensure we're testing all expected relationships
assert len(_ALL_RELATIONSHIPS) == 10, f"Expected 10 relationships, got {len(_ALL_RELATIONSHIPS)}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_inspector.py` around lines 79 - 81, Add a sanity check
assertion immediately after the `_ALL_RELATIONSHIPS` list comprehension to
verify the expected number of relationships are present. This assertion should
check that the length of `_ALL_RELATIONSHIPS` equals the expected count (10) and
fail with a descriptive message if the count is incorrect, catching any
accidental model removals or broken relationship definitions that would
otherwise cause silent test failures.
1 participant
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by CodeRabbit
Bug Fixes
Tests