Skip to content

refactor(migrations): reduce instance attributes across migration helpers - #835

Open
renansuaris wants to merge 3 commits into
redis:mainfrom
renansuaris:refator/too-many-instance-attributes
Open

refactor(migrations): reduce instance attributes across migration helpers#835
renansuaris wants to merge 3 commits into
redis:mainfrom
renansuaris:refator/too-many-instance-attributes

Conversation

@renansuaris

@renansuaris renansuaris commented May 29, 2026

Copy link
Copy Markdown

What does this PR do?

Refactors the migration-related classes to reduce the number of instance attributes and satisfy Pylint's R0902 warning ("too-many-instance-attributes"). The changes extract or remove redundant state so each class keeps only the data it actually needs.

Why is this change needed?

Some migration classes were accumulating execution state, progress tracking, and metrics in the same object, which made them harder to read, test, and maintain. Reducing instance attributes improves cohesion, simplifies the class responsibilities, and keeps the code within the project's linting rules without changing external behavior.

How was it tested?

Existing tests were run and continue to pass.

Notes for reviewers

This refactor is intentionally minimal and isolated to the instances flagged by Pylint. The public behavior of the migrations should remain unchanged.


Note

Medium Risk
Touches central query execution, RediSearch query building, and model metaclass/index schema logic; behavior is intended to be unchanged but regressions would affect most ORM usage.

Overview
Refactors migration and core model/query classes so execution state, caches, and metadata live in smaller helper objects or properties instead of many loose instance attributes, addressing Pylint R0902 while keeping outward APIs the same.

In datetime hash migration, redundant resume/legacy counters (enable_resume, _processed_keys) are removed, and hash scanning/conversion is moved into _collect_hash_keys, _normalize_hash_data, and _process_hash_key used by _process_hash_model.

DataMigrator.run_migrations_with_monitoring now builds and logs results via _log_pending_migrations, _build_monitoring_result, and _log_monitoring_summary instead of inlined dict/print blocks.

model.py is the bulk of the change: FindQuery stores config in _FindQueryState and memoized pieces in _FindQueryCache (with property shims); RediSearch value resolution and execute() are split into focused helpers; FieldInfo Redis options and VectorFieldOptions optional params are nested behind properties; DefaultMeta uses ClassVars; ModelMeta.__new__ and JSON index schema_for_type recursion are decomposed into static/class helpers. A test asserts limit=0 still resolves pagination to the default page size (1000).

Reviewed by Cursor Bugbot for commit 90d8084. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread aredis_om/model/model.py Outdated
Comment thread aredis_om/model/model.py

@property
def _model_cache(self):
return self._cache.model_cache

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary indirection wrapping attributes in dataclass containers

Low Severity

The FindQuery refactoring introduces _FindQueryState and _FindQueryCache dataclasses plus ~15 property definitions (getters and setters) that add ~120 lines of boilerplate indirection. The attributes still live on the same object conceptually — they're just accessed through an extra layer. This doesn't improve cohesion or testability; it only suppresses the lint warning while making the class harder to read and maintain.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1c3a4b2. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 90d8084. Configure here.

monitor=monitor,
errors=[],
dry_run=True,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dry-run result gains unintended extra success_rate field

Low Severity

The dry-run path now calls _build_monitoring_result, which unconditionally adds a success_rate key to the returned dict. Previously, the dry-run result only contained applied_count, total_migrations, performance_stats, errors, and dry_run. Any consumers or tests asserting on the exact shape of the dry-run result dict will break. The PR states "public behavior should remain unchanged," but this changes the return value contract.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 90d8084. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants