- Notifications
You must be signed in to change notification settings - Fork112
WIP: Custom frozen dataclasses#586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
sourcery-aibot commentedFeb 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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: DecoratesFile-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess yourdashboard to:
Getting Help
|
codecovbot commentedFeb 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## master #586 +/- ##==========================================+ Coverage 79.83% 80.25% +0.42%========================================== Files 22 25 +3 Lines 1914 2117 +203 Branches 294 338 +44 ==========================================+ Hits 1528 1699 +171- Misses 266 288 +22- Partials 120 130 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d4dc6e7 toa490526Comparetony commentedFeb 28, 2025
@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
_frozenattribute.
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 to70e415cCompare4353421 toe18198bComparee77bc7d toe881b35Comparee881b35 toba0a3b9Compareab84634 to5803841Comparewhy: Fix type checking errors in the custom frozen_dataclass implementationwhat:- 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 codebaserefs: Enables inheritance from mutable to immutable dataclasses
…thod-assign`why: Fix type checking errors in the custom frozen_dataclass implementationwhat:- 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 codebaserefs: Enables inheritance from mutable to immutable dataclasses
…iles from type checkingwhy: 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 featuresrefs: This addresses the mypy test failures while maintaining type safety for the implementation
…tests from strict checkingwhy:The frozen_dataclass_sealable decorator adds attributes and methods dynamicallyat runtime which causes false positive errors with static analysis tools.Testing this functionality requires patterns that deliberately violate somerules.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 featuresrefs: This maintains code quality while acknowledging the inherentlimitations of static analysis tools when dealing with Python's dynamic runtimefeatures
ba0a3b9 to0975cfdCompare
Uh oh!
There was an error while loading.Please reload this page.
Resolves#588
Summary by Sourcery
Tests:
frozen_dataclassdecorator, covering initialization, immutability, inheritance, edge cases, nested mutability, bidirectional references, and mutation methods.