-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
WIP: Custom frozen dataclasses #586
base: master
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces a Class diagram for frozen_dataclass decoratorclassDiagram
class frozen_dataclass {
+__init__(cls: type[_T]) type[_T]
}
class dataclasses.dataclass {
+__init__(frozen: bool)
}
class cls {
+__setattr__(name: str, value: t.Any) None
+__delattr__(name: str) None
-_frozen: bool
}
frozen_dataclass --|> dataclasses.dataclass: Uses
frozen_dataclass --|> cls: Decorates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #586 +/- ##
==========================================
+ Coverage 81.48% 81.69% +0.21%
==========================================
Files 37 40 +3
Lines 2430 2633 +203
Branches 368 412 +44
==========================================
+ Hits 1980 2151 +171
- Misses 308 330 +22
- Partials 142 152 +10 ☔ View full report in Codecov by Sentry. |
d4dc6e7
to
a490526
Compare
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a note about the performance implications of using
__setattr__
and__delattr__
. - It would be helpful to include a warning in the documentation about the potential for users to bypass the immutability by modifying the
_frozen
attribute.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
width=80, | ||
height=24, | ||
expected_error=False, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (testing): Edge case: Unfreezing via _frozen attribute
Although tested, the ability to unfreeze by modifying _frozen
is a security risk. Consider making _frozen
a property with a private setter to prevent this or document this behavior clearly as a known limitation.
d2c13aa
to
70e415c
Compare
4353421
to
e18198b
Compare
e77bc7d
to
e881b35
Compare
why: Fix type checking errors in the custom frozen_dataclass implementation what: - Added targeted mypy configuration override to disable method-assign errors - Only scoped to libtmux._internal.frozen_dataclass module - Preserves strict type checking across the rest of the codebase refs: Enables inheritance from mutable to immutable dataclasses
…thod-assign` why: Fix type checking errors in the custom frozen_dataclass implementation what: - Added targeted mypy configuration override to disable method-assign errors - Only scoped to libtmux._internal.frozen_dataclass module - Preserves strict type checking across the rest of the codebase refs: Enables inheritance from mutable to immutable dataclasses
…iles from type checking why: The frozen_dataclass_sealable decorator adds attributes and methods dynamically at runtime, which mypy cannot properly analyze in test contexts, resulting in false positive errors. what: - Added mypy override to ignore type errors in tests._internal.test_frozen_dataclass_sealable - Added mypy override to ignore type errors in tests.examples._internal.frozen_dataclass_sealable.test_basic - Preserves strict typing for the implementation code while allowing tests to use dynamic features refs: This addresses the mypy test failures while maintaining type safety for the implementation
…tests from strict checking why: The frozen_dataclass_sealable decorator adds attributes and methods dynamically at runtime which causes false positive errors with static analysis tools. Testing this functionality requires patterns that deliberately violate some rules. what: - Added mypy override to ignore type errors in tests._internal.test_frozen_dataclass_sealable - Added mypy override to ignore type errors in tests.examples._internal.frozen_dataclass_sealable.test_basic - Added per-file ignore for RUF009 (function call in default argument) in test_frozen_dataclass_sealable.py - Preserves strict typing and linting for implementation code while allowing tests to use dynamic features refs: This maintains code quality while acknowledging the inherent limitations of static analysis tools when dealing with Python's dynamic runtime features
e881b35
to
ba0a3b9
Compare
Resolves #588
Summary by Sourcery
Tests:
frozen_dataclass
decorator, covering initialization, immutability, inheritance, edge cases, nested mutability, bidirectional references, and mutation methods.