Conversation
📝 WalkthroughWalkthroughAdds granular controls for dataclass field updates: Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as Client API (aset / at[].set / aset_inplace)
participant Validator as Field Validator
participant Copier as _copy_and_set / Inplace Mutator
participant Instance as DataClass Instance
participant Callbacks as DRINX_ON_SETATTR
User->>API: request update(field, value, allow_private?, bypass_callbacks?, create_new_ok?)
API->>Validator: verify field exists / check create_new_ok and init flag
alt Field missing & create_new_ok=False
Validator-->>API: raise TypeError
API-->>User: error
else Field present or creation allowed
Validator->>Validator: determine if init=False and whether allow_private allows it
alt init=False and allow_private=False
Validator-->>API: raise TypeError
API-->>User: error
else
API->>Callbacks: if not bypass -> run pre/field callbacks (may be nested)
API->>Copier: perform update (functional via _copy_and_set or inplace mutator)
Copier->>Instance: clone+set or mutate, apply callbacks if not already run
Instance-->>Copier: updated instance/state
Copier-->>API: return result
API-->>User: return updated instance or None (inplace)
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
==========================================
- Coverage 86.47% 85.96% -0.51%
==========================================
Files 6 6
Lines 451 506 +55
Branches 96 114 +18
==========================================
+ Hits 390 435 +45
- Misses 39 46 +7
- Partials 22 25 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 the current code and only fix it if needed.
Inline comments:
In `@src/drinx/base.py`:
- Around line 377-385: The _copy_and_set helper bypasses on_setattr callbacks
(and any __post_init__ logic) because it creates a shallow copy via
object.__new__ and object.__setattr__, so updates performed through aset no
longer trigger callbacks as dataclasses.replace did; fix by either restoring
behavior: call the same callback invocation used by on_setattr (or reuse
dataclasses.replace) when setting field_name in _copy_and_set (ensure
__post_init__/on_setattr handlers are invoked for the new value), or explicitly
document that _copy_and_set intentionally skips callbacks; reference
functions/classes: _copy_and_set, aset, on_setattr, dataclasses.replace, and
__post_init__ when applying the chosen change.
- Around line 377-385: The _copy_and_set function fails for slots-based
dataclass instances because it assumes obj.__dict__ exists; update _copy_and_set
to detect if the instance has a __dict__ and copy attributes from it, and
otherwise iterate over the class's __slots__ (and any inherited slots) to read
and copy slot values, using object.__getattribute__ to fetch each attribute and
object.__setattr__ to set them on the newly created instance; preserve the
existing behavior of setting the target field_name after copying and ensure
private/ mangled slot names are handled (e.g., look up slot names on type(obj)
and skip slot names not present on the instance).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 493bd431-f0bb-4e0a-8598-7b31f3b74317
📒 Files selected for processing (4)
src/drinx/base.pysrc/drinx/transform.pysrc/drinx/visualize.pytests/test_base.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/drinx/base.py (1)
431-436:⚠️ Potential issue | 🔴 Critical
allow_private=Truestill rejects declared private fields that have not been initialized yet.This check uses
hasattr(parent, str(final_op))before you look at the dataclass field definition. Forprivate_field()/init=Falsefields without a default ordefault_factory, the attribute is absent from the instance, soaset("_field", ..., allow_private=True)still raises here instead of reaching the new private-field path.Suggested fix
if final_op_type == "attribute": - if not hasattr(parent, str(final_op)): - if not create_new_ok: - raise Exception( - f"Attribute: {final_op} does not exist for {parent.__class__}" - ) + if dataclasses.is_dataclass(parent): + dc_field_names = {f.name for f in dataclasses.fields(parent)} + if str(final_op) not in dc_field_names and not create_new_ok: + raise Exception( + f"Attribute: {final_op} does not exist for {parent.__class__}" + ) + elif not hasattr(parent, str(final_op)) and not create_new_ok: + raise Exception( + f"Attribute: {final_op} does not exist for {parent.__class__}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drinx/base.py` around lines 431 - 436, The current attribute-existence check in the final_op_type == "attribute" branch uses hasattr(parent, str(final_op)) which incorrectly raises for declared dataclass private fields (private_field/init=False without default) that are not yet set; update the logic to, when hasattr returns False and create_new_ok is False, inspect the dataclass definition on type(parent) (e.g., type(parent).__dataclass_fields__ or dataclasses.fields(type(parent))) to see if a field named str(final_op) is declared and represents a private/init=False field and allow_private is True — if so, do not raise and let the existing private-field handling path proceed; otherwise keep raising the exception referring to final_op and parent.__class__. Ensure you reference final_op_type, parent, final_op, create_new_ok and the allow_private/private_field semantics when implementing the check.
♻️ Duplicate comments (1)
src/drinx/base.py (1)
381-389:⚠️ Potential issue | 🟠 Major
asetnow bypasses__post_init__invariants for functional updates.
_copy_and_setclones__dict__and writes the field directly, so any derived/cache fields recomputed in__post_init__stay stale afteraset. That is a behavior regression fromdataclasses.replace(...)and can leave the returned object internally inconsistent.One way to preserve the old semantics for normal init-fields
- cur_attr = DataClass._copy_and_set(current_parent, str(op), cur_attr) + if target_field.init and not bypass_callbacks: + cur_attr = dataclasses.replace( + current_parent, **{str(op): cur_attr} + ) + else: + cur_attr = DataClass._copy_and_set( + current_parent, str(op), cur_attr + )Also applies to: 470-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drinx/base.py` around lines 381 - 389, _copy_and_set currently clones __dict__ and writes the field directly which bypasses dataclass/attrs post-init invariants; fix by preserving semantics: if the object is a dataclass use dataclasses.replace(obj, **{field_name: value}) inside _copy_and_set (or in aset), otherwise after copying and setting the new attribute call the class/object post-init hook if present (e.g., post = getattr(new_obj, "__post_init__", None); if callable(post): post()) so derived/cache fields are recomputed; update the same logic for the other occurrence around lines 470-475 (reference functions: _copy_and_set and aset).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/drinx/base.py`:
- Around line 83-90: The docstring says weakref_slot is ignored but the code
still forwards weakref_slot to the stdlib dataclass decorator, which raises if
weakref_slot=True while slots isn't passed; update the implementation so
weakref_slot is discarded like slots instead of being forwarded to dataclass:
locate the argument handling where slots and weakref_slot are received and the
decorator application around the dataclass(...) call (references: variables
slots and weakref_slot and the dataclass decorator invocation that wraps the
class), remove or omit weakref_slot from the keyword args passed into
dataclass(...) and ensure the decorator call no longer forwards weakref_slot to
avoid runtime validation errors.
---
Outside diff comments:
In `@src/drinx/base.py`:
- Around line 431-436: The current attribute-existence check in the
final_op_type == "attribute" branch uses hasattr(parent, str(final_op)) which
incorrectly raises for declared dataclass private fields
(private_field/init=False without default) that are not yet set; update the
logic to, when hasattr returns False and create_new_ok is False, inspect the
dataclass definition on type(parent) (e.g., type(parent).__dataclass_fields__ or
dataclasses.fields(type(parent))) to see if a field named str(final_op) is
declared and represents a private/init=False field and allow_private is True —
if so, do not raise and let the existing private-field handling path proceed;
otherwise keep raising the exception referring to final_op and parent.__class__.
Ensure you reference final_op_type, parent, final_op, create_new_ok and the
allow_private/private_field semantics when implementing the check.
---
Duplicate comments:
In `@src/drinx/base.py`:
- Around line 381-389: _copy_and_set currently clones __dict__ and writes the
field directly which bypasses dataclass/attrs post-init invariants; fix by
preserving semantics: if the object is a dataclass use dataclasses.replace(obj,
**{field_name: value}) inside _copy_and_set (or in aset), otherwise after
copying and setting the new attribute call the class/object post-init hook if
present (e.g., post = getattr(new_obj, "__post_init__", None); if
callable(post): post()) so derived/cache fields are recomputed; update the same
logic for the other occurrence around lines 470-475 (reference functions:
_copy_and_set and aset).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f122fa3-a111-48c6-8238-43a13401dcc0
📒 Files selected for processing (3)
src/drinx/base.pysrc/drinx/transform.pytests/test_base.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/drinx/transform.py`:
- Around line 145-149: The docstring for the dataclass options mentions a
`weakref_slot` that "Add a ``__weakref__`` slot", but at runtime the
`weakref_slot` parameter is ignored and forced to False (so it does not add a
__weakref__ slot); update the docstring text around the `slots` / `weakref_slot`
entries to state that `weakref_slot` is not supported/ignored (or deprecated)
and has no effect (defaults to False) instead of claiming it adds a __weakref__
slot; reference the `weakref_slot` parameter name in your change and remove or
replace the misleading sentence so the docs match the code behavior in
transform.py.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a5f6b47-e96a-4d2c-adee-e44c1ccc6e4d
📒 Files selected for processing (3)
src/drinx/base.pysrc/drinx/transform.pytests/test_dataclass.py
💤 Files with no reviewable changes (1)
- tests/test_dataclass.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/drinx/base.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_base.py (1)
2175-2181: Minor: Clarify comment wording.The comment "no callback: value unchanged" at line 2181 is slightly misleading—the value IS changed (from 1.0 to 9.0), it's just not transformed by any callback. Consider: "no callback registered: value set as-is".
📝 Suggested comment clarification
- assert foo.x == 9.0 # no callback: value unchanged + assert foo.x == 9.0 # no callback registered: value set as-is🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_base.py` around lines 2175 - 2181, Update the inline test comment in test_bypass_false_no_callback_registered to clearly state that the field value was changed but not transformed by any callback; replace "no callback: value unchanged" with something like "no callback registered: value set as-is" (refer to the test function test_bypass_false_no_callback_registered, the Foo DataClass, and the aset_inplace call) so the comment accurately reflects that the value was set to 9.0 without callback processing.src/drinx/base.py (1)
618-634: Docstring missingallow_privateparameter documentation.The
setmethod signature includesallow_private: bool = Falseat line 619, but the docstring Args section (lines 628-630) only documentsbypass_callbacks. Theallow_privateparameter should be documented for API completeness.📝 Add missing parameter documentation
Args: value: The new value. For mask updates this may be a scalar or a DataClass tree of the same type as the mask/object. + allow_private: If True, allow updating non-init (private) fields. + Default False rejects such updates. bypass_callbacks: If True, skip ``on_setattr`` callbacks. Default False runs them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drinx/base.py` around lines 618 - 634, The docstring for DataClass.set is missing documentation for the allow_private parameter; update the Args section of the set method's docstring to include allow_private: bool = False and a short description (e.g., whether private attributes/keys are allowed to be set or filtered out), mirroring the style used for bypass_callbacks and mentioning interaction with DataClass.aset or mask updates where relevant so callers understand its effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/drinx/base.py`:
- Around line 618-634: The docstring for DataClass.set is missing documentation
for the allow_private parameter; update the Args section of the set method's
docstring to include allow_private: bool = False and a short description (e.g.,
whether private attributes/keys are allowed to be set or filtered out),
mirroring the style used for bypass_callbacks and mentioning interaction with
DataClass.aset or mask updates where relevant so callers understand its effect.
In `@tests/test_base.py`:
- Around line 2175-2181: Update the inline test comment in
test_bypass_false_no_callback_registered to clearly state that the field value
was changed but not transformed by any callback; replace "no callback: value
unchanged" with something like "no callback registered: value set as-is" (refer
to the test function test_bypass_false_no_callback_registered, the Foo
DataClass, and the aset_inplace call) so the comment accurately reflects that
the value was set to 9.0 without callback processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0e89ece-b743-4c73-bfaa-f5a2b1dc0243
📒 Files selected for processing (2)
src/drinx/base.pytests/test_base.py
Merged
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
2 participants
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.
resolves #9
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests