Ruff rules S, SIM, SLOT#126
Open
cclauss wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughRefactors simplify conditional and membership expressions across engine, utilities, and tests, add a document-type guard before document comparisons, replace a try/except with contextlib.suppress, and expand Ruff lint rules; behavior and public interfaces are unchanged. ChangesCode Simplification and Quality Improvements
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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
montydb/engine/weighted.py (1)
87-93:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
__slots__incompatibility that preventsWeightedinstantiation.The
__slots__ = ()declaration on the tuple subclass prevents theself.value = valueassignment in__init__. WhenWeighted(v)is instantiated (line 567 incollection.py), it will raiseAttributeError: 'Weighted' object has no attribute 'value'. This breaks thedistinct()method which depends on accessing.valueat line 583.Either remove the
__slots__ = ()declaration or remove theself.value = valueassignment. If the value storage is necessary, consider an alternative approach that doesn't conflict with the empty slots.🤖 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 `@montydb/engine/weighted.py` around lines 87 - 93, The Weighted tuple subclass currently sets __slots__ = () which prevents assigning an instance attribute in __init__ and causes AttributeError when instantiating Weighted(value); fix by removing or altering the __slots__ declaration so instance attribute storage is allowed, or by eliminating self.value and instead rely on the tuple contents (e.g., access the stored value via tuple indexing) — update the class Weighted (its __slots__, __new__, and __init__ methods and usages of .value) and ensure callers (e.g., distinct()) read the value from the adjusted representation (either instance attribute or tuple element) consistently.
🧹 Nitpick comments (1)
pyproject.toml (1)
71-71: ⚡ Quick winConsider scoping
S101to test files only.
S101warns against usingassertstatements because they can be optimized away withpython -O. Currently, it's globally ignored, which meansassertstatements are allowed throughout the codebase, including production code.Consider moving
S101toper-file-ignoresto allow asserts only in test files:🔒 Suggested configuration to scope S101 to tests
ignore = [ - "S101", "SIM102", "UP015", ] per-file-ignores = { "montydb/storage/sqlite.py" = [ "SIM117" ], + "tests/**/*.py" = [ "S101" ], }🤖 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 `@pyproject.toml` at line 71, The project currently ignores the S101 Flake8 rule globally in pyproject.toml (entry "S101"), which allows use of assert in production; update pyproject.toml to remove "S101" from the global ignores and instead add it under per-file-ignores scoped to your test files (e.g., add an entry under [tool.flake8].per-file-ignores mapping test/* or tests/* to "S101") so asserts remain allowed only in test modules; ensure the per-file-ignores key and the exact pattern (tests/* or test_*.py) match your test layout and keep other existing ignores intact.
🤖 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.
Outside diff comments:
In `@montydb/engine/weighted.py`:
- Around line 87-93: The Weighted tuple subclass currently sets __slots__ = ()
which prevents assigning an instance attribute in __init__ and causes
AttributeError when instantiating Weighted(value); fix by removing or altering
the __slots__ declaration so instance attribute storage is allowed, or by
eliminating self.value and instead rely on the tuple contents (e.g., access the
stored value via tuple indexing) — update the class Weighted (its __slots__,
__new__, and __init__ methods and usages of .value) and ensure callers (e.g.,
distinct()) read the value from the adjusted representation (either instance
attribute or tuple element) consistently.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 71: The project currently ignores the S101 Flake8 rule globally in
pyproject.toml (entry "S101"), which allows use of assert in production; update
pyproject.toml to remove "S101" from the global ignores and instead add it under
per-file-ignores scoped to your test files (e.g., add an entry under
[tool.flake8].per-file-ignores mapping test/* or tests/* to "S101") so asserts
remain allowed only in test modules; ensure the per-file-ignores key and the
exact pattern (tests/* or test_*.py) match your test layout and keep other
existing ignores intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8debbaa1-0f8e-4d7e-b8f5-01c778aaab23
📒 Files selected for processing (13)
montydb/engine/field_walker.pymontydb/engine/project.pymontydb/engine/queries.pymontydb/engine/update.pymontydb/engine/weighted.pymontydb/types/objectid.pymontydb/utils/io.pypyproject.tomltests/test_client.pytests/test_database.pytests/test_engine/conftest.pytests/test_engine/test_fieldwalker/test_fieldwalker_set.pytests/test_packaging.py
cclauss
force-pushed
the
ruff-rules-S-SIM-SLOT
branch
from
f164f21 to
83a7209
Compare
cclauss
force-pushed
the
ruff-rules-S-SIM-SLOT
branch
from
83a7209 to
5323011
Compare
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 by CodeRabbit
Bug Fixes
Refactor
Tests
Chores