chore: improve token string representations #1307
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (80.00%) is below the target coverage (92.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #1307 +/- ##
==========================================
- Coverage 92.29% 92.16% -0.14%
==========================================
Files 139 140 +1
Lines 8515 8601 +86
==========================================
+ Hits 7859 7927 +68
- Misses 656 674 +18 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces automatic string representation generation for token-related dataclasses, eliminating the need for manual __str__ and __repr__ maintenance. The changes implement a new utility module that dynamically generates string representations based on dataclass fields, ensuring new fields are automatically included without code updates.
Key changes:
- Add
DataclassStringMixinand@auto_str_reprdecorator for automatic string generation - Apply the mixin to token classes (
TokenUpdateParams,TokenUpdateKeys,TokenRelationship) - Add custom
__str__implementation forCustomFeebase class using dynamic attribute inspection
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/hiero_sdk_python/utils/dataclass_strings.py |
New utility module providing DataclassStringMixin and @auto_str_repr decorator for automatic string generation |
tests/unit/dataclass_strings_test.py |
Comprehensive test suite covering mixin/decorator functionality, formatting rules, and integration with token classes |
src/hiero_sdk_python/tokens/token_update_transaction.py |
Apply DataclassStringMixin to TokenUpdateParams and TokenUpdateKeys classes |
src/hiero_sdk_python/tokens/token_relationship.py |
Apply DataclassStringMixin to TokenRelationship class |
src/hiero_sdk_python/tokens/custom_fee.py |
Add dynamic __str__ and __repr__ methods to CustomFee base class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not dataclasses.is_dataclass(cls): | ||
| raise TypeError(f"@auto_str_repr can only be applied to dataclasses, got {cls.__name__}") | ||
|
|
||
| def _format_value(value: Any) -> str: |
There was a problem hiding this comment.
The _format_value function duplicates logic from DataclassStringMixin._format_field_value. Consider extracting this shared formatting logic into a standalone function to reduce duplication and improve maintainability.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a dataclass-driven string utility (DataclassStringMixin and Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/hiero_sdk_python/tokens/custom_fee.pysrc/hiero_sdk_python/tokens/token_relationship.pysrc/hiero_sdk_python/tokens/token_update_transaction.pysrc/hiero_sdk_python/utils/dataclass_strings.pytests/unit/dataclass_strings_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/dataclass_strings_test.py
🧬 Code graph analysis (2)
src/hiero_sdk_python/tokens/token_update_transaction.py (1)
src/hiero_sdk_python/utils/dataclass_strings.py (1)
DataclassStringMixin(29-122)
src/hiero_sdk_python/tokens/token_relationship.py (1)
src/hiero_sdk_python/utils/dataclass_strings.py (1)
DataclassStringMixin(29-122)
🪛 GitHub Check: Codacy Static Code Analysis
src/hiero_sdk_python/utils/dataclass_strings.py
[warning] 80-80: src/hiero_sdk_python/utils/dataclass_strings.py#L80
Method _format_field_value has a cyclomatic complexity of 9 (limit is 8)
[warning] 151-151: src/hiero_sdk_python/utils/dataclass_strings.py#L151
Method auto_str_repr._format_value has a cyclomatic complexity of 9 (limit is 8)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/utils/dataclass_strings.py
26-26: typing.Dict is deprecated, use dict instead
(UP035)
80-80: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
tests/unit/dataclass_strings_test.py
200-200: Possible hardcoded password assigned to argument: "token_name"
(S106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Agent
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.10)
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (4)
src/hiero_sdk_python/utils/dataclass_strings.py (1)
57-74: LGTM! Clean implementation with proper guards.The dynamic field introspection and conditional formatting (single-line for ≤3 fields, multi-line for >3) provide a good balance between readability and conciseness. The defensive check for non-dataclass usage is appropriate.
src/hiero_sdk_python/tokens/token_update_transaction.py (1)
26-26: LGTM! Mixin integration is clean and consistent.The addition of
DataclassStringMixinto bothTokenUpdateParamsandTokenUpdateKeyscorrectly applies the automatic string representation pattern. These dataclasses now benefit from zero-maintenance string methods that will automatically include any future field additions.Also applies to: 29-29, 50-50
src/hiero_sdk_python/tokens/token_relationship.py (1)
19-19: LGTM! Consistent mixin adoption.The integration of
DataclassStringMixinintoTokenRelationshipfollows the same clean pattern as the other token classes. The automatic string generation will now include all fields (token_id, symbol, balance, kyc_status, freeze_status, decimals, automatic_association) without manual maintenance.Also applies to: 22-22
tests/unit/dataclass_strings_test.py (1)
55-260: Test coverage is comprehensive and follows best practices.The test suite demonstrates:
- ✅ Clear, descriptive test names
- ✅ No print statements or timing dependencies
- ✅ Real SDK imports (no hallucinated modules)
- ✅ Comprehensive coverage of mixin, decorator, and integration scenarios
- ✅ Tests for edge cases (None handling, long bytes truncation, enum formatting)
The static analysis hint about "hardcoded password" on line 200 is a false positive—
token_name="Updated Token"is a token name, not a credential.
| class TestDynamicFieldInclusion: | ||
| """Tests verifying that new fields are automatically included.""" | ||
|
|
||
| def test_new_fields_automatically_included(self): | ||
| """Verify that adding fields doesn't require __str__ updates.""" | ||
| @dataclass | ||
| class ExtendableClass(DataclassStringMixin): | ||
| original_field: str | ||
| # Simulate adding a new field | ||
| new_field: Optional[str] = None | ||
| another_new_field: int = 0 | ||
|
|
||
| obj = ExtendableClass( | ||
| original_field="original", | ||
| new_field="new", | ||
| another_new_field=42 | ||
| ) | ||
|
|
||
| result = str(obj) | ||
|
|
||
| # All fields should be included without any code changes | ||
| assert "original_field='original'" in result | ||
| assert "new_field='new'" in result | ||
| assert "another_new_field=42" in result |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider clarifying test name and intent.
The test name test_new_fields_automatically_included might suggest runtime field addition, but dataclass fields are fixed at decoration time. The test actually validates that when developers add fields to a dataclass definition, they don't need to update __str__ methods manually—which is the correct intent.
Consider renaming to be more explicit:
def test_dataclass_definition_changes_require_no_str_updates(self):
"""Verify that adding fields to dataclass definition doesn't require __str__ updates.
This simulates the maintenance benefit: when a developer adds new fields
to a dataclass, the string representation automatically includes them
without manual changes to __str__/__repr__.
"""Alternatively, you could add a comment explaining the test's purpose more clearly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
10-10: Spaces inside code span elements
(MD038, no-space-in-code)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.13)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
tech0priyanshu
force-pushed
the
feat/auto-str
branch
from
b863014 to
3e51092
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/hiero_sdk_python/utils/dataclass_strings.py (3)
25-26: Use built-indictinstead of deprecatedtyping.Dict.Python 3.9+ supports the built-in
dicttype for type hints. Thetyping.Dictimport is deprecated.🔎 Proposed fix
import dataclasses -from typing import Any, Dict +from typing import AnyAnd update line 104:
- def to_dict(self) -> Dict[str, Any]: + def to_dict(self) -> dict[str, Any]:Based on static analysis hints and past review comments.
96-99: Simplify redundanthasattrchecks.All Python objects have
__class__, and all classes have__mro__. These checks are unnecessary and can be safely removed.🔎 Proposed simplification
# Handle enum values - show EnumClass.VALUE format - if hasattr(value, '__class__') and hasattr(value.__class__, '__mro__'): - for base in value.__class__.__mro__: - if base.__name__ == 'Enum': - return f"{value.__class__.__name__}.{value.name}" + for base in value.__class__.__mro__: + if base.__name__ == 'Enum': + return f"{value.__class__.__name__}.{value.name}"Based on past review comments.
125-188: Extract duplicated formatting logic into a shared module-level function.The
_format_valuefunction (lines 151-165) duplicates the formatting logic fromDataclassStringMixin._format_field_value(lines 80-102). This creates significant maintenance burden—any changes to formatting behavior must be applied in multiple places, andCustomFeealso duplicates this logic.🔎 Proposed refactor to eliminate duplication
Extract the formatting logic into a module-level function that all implementations can share:
+def _format_field_value(value: Any) -> str: + """Format a field value for string representation. + + Shared formatting logic used by DataclassStringMixin, auto_str_repr, + and other classes that need consistent field formatting. + """ + if value is None: + return "None" + if isinstance(value, str): + return f"'{value}'" + if isinstance(value, bytes): + if len(value) > 20: + return f"b'{value[:20].hex()}...'" + return f"b'{value.hex()}'" + for base in value.__class__.__mro__: + if base.__name__ == 'Enum': + return f"{value.__class__.__name__}.{value.name}" + return str(value) + + class DataclassStringMixin: # ... existing code ... def _format_field_value(self, value: Any) -> str: """Format a field value for string representation.""" - if value is None: - return "None" - # ... rest of logic ... - return str(value) + return _format_field_value(value) # Delegate to shared function def auto_str_repr(cls): # ... validation ... - def _format_value(value: Any) -> str: - """Format a field value for string representation.""" - if value is None: - return "None" - # ... duplicated logic ... - return str(value) - def __str__(self) -> str: """Generate string representation dynamically from dataclass fields.""" field_strings = [] for field in dataclasses.fields(self.__class__): field_value = getattr(self, field.name) - formatted_value = _format_value(field_value) + formatted_value = _format_field_value(field_value) # Use shared function field_strings.append(f"{field.name}={formatted_value}") # ... rest of __str__ ...This shared function can then also be used by
CustomFeeto eliminate its third copy of this logic.Based on past review comments.
CHANGELOG.md (1)
10-10: Fix markdown formatting in code span.There is a trailing space inside the code span. The entry should have
`__str__`(not`__str__ `) to comply with markdown linting rules (MD038).🔎 Proposed fix
-- Refactored token classes to use dataclass-driven `__str__ `and `__repr__` generation. [#999](https://github.com/hiero-ledger/hiero-sdk-python/issues/999) +- Refactored token classes to use dataclass-driven `__str__` and `__repr__` generation. [#999](https://github.com/hiero-ledger/hiero-sdk-python/issues/999)Based on static analysis hints and past review comments.
src/hiero_sdk_python/tokens/custom_fee.py (1)
153-177: Extract shared formatting logic to eliminate third instance of duplication.The formatting logic in
__str__(lines 159-166) is the third copy of identical formatting rules—the same logic exists inDataclassStringMixin._format_field_valueandauto_str_repr._format_value. This creates significant maintenance burden.Additionally, using
__dict__.items()directly may include private attributes (starting with_) or internal implementation details that shouldn't be in string representations.🔎 Proposed refactor using shared formatting function
After extracting the shared formatting function in
dataclass_strings.py(as suggested in that file's review), update this implementation:+from hiero_sdk_python.utils.dataclass_strings import _format_field_value def __str__(self) -> str: - """Return a dynamic string representation including all instance attributes.""" + """Return a dynamic string representation including all public instance attributes.""" fields = [] for key, value in self.__dict__.items(): - if value is None: - fields.append(f"{key}=None") - elif isinstance(value, str): - fields.append(f"{key}='{value}'") - else: - fields.append(f"{key}={value}") + # Skip private or internal attributes + if key.startswith("_"): + continue + formatted_value = _format_field_value(value) + fields.append(f"{key}={formatted_value}") class_name = self.__class__.__name__ if len(fields) <= 3: return f"{class_name}({', '.join(fields)})" else: fields_str = ",\n ".join(fields) return f"{class_name}(\n {fields_str}\n)"This also filters out private attributes (those starting with
_) to avoid exposing internal state.Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdsrc/hiero_sdk_python/tokens/custom_fee.pysrc/hiero_sdk_python/tokens/token_relationship.pysrc/hiero_sdk_python/tokens/token_update_transaction.pysrc/hiero_sdk_python/utils/dataclass_strings.pytests/unit/dataclass_strings_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/dataclass_strings_test.py
🧬 Code graph analysis (3)
src/hiero_sdk_python/tokens/token_update_transaction.py (1)
src/hiero_sdk_python/utils/dataclass_strings.py (1)
DataclassStringMixin(29-122)
src/hiero_sdk_python/tokens/token_relationship.py (1)
src/hiero_sdk_python/utils/dataclass_strings.py (1)
DataclassStringMixin(29-122)
tests/unit/dataclass_strings_test.py (1)
src/hiero_sdk_python/utils/dataclass_strings.py (3)
DataclassStringMixin(29-122)auto_str_repr(125-188)to_dict(104-122)
🪛 GitHub Check: Codacy Static Code Analysis
src/hiero_sdk_python/utils/dataclass_strings.py
[warning] 80-80: src/hiero_sdk_python/utils/dataclass_strings.py#L80
Method _format_field_value has a cyclomatic complexity of 9 (limit is 8)
[warning] 151-151: src/hiero_sdk_python/utils/dataclass_strings.py#L151
Method auto_str_repr._format_value has a cyclomatic complexity of 9 (limit is 8)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
10-10: Spaces inside code span elements
(MD038, no-space-in-code)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/utils/dataclass_strings.py
26-26: typing.Dict is deprecated, use dict instead
(UP035)
80-80: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
tests/unit/dataclass_strings_test.py
200-200: Possible hardcoded password assigned to argument: "token_name"
(S106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.10)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (3)
src/hiero_sdk_python/tokens/token_relationship.py (1)
19-22: LGTM! Clean integration of DataclassStringMixin.The changes correctly integrate the
DataclassStringMixinto provide automatic string representations for theTokenRelationshipdataclass. This aligns with the PR's goal of eliminating manual maintenance of__str__and__repr__methods.src/hiero_sdk_python/tokens/token_update_transaction.py (1)
26-26: LGTM! Clean integration of DataclassStringMixin for both dataclasses.The changes correctly integrate the
DataclassStringMixininto bothTokenUpdateParamsandTokenUpdateKeysdataclasses, providing automatic string representations. This aligns with the PR's goal of eliminating manual maintenance of__str__and__repr__methods across token-related classes.Also applies to: 29-29, 50-50
tests/unit/dataclass_strings_test.py (1)
193-206: Static analysis false positive - ignore.The Ruff warning on line 200 about a "hardcoded password" is a false positive. The variable
token_namerefers to a token identifier, not an authentication credential. This is safe to ignore.
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@tech0priyanshu
This is a solid refactor overall and a clear improvement over the existing manual str / repr implementations. The introduction of DataclassStringMixin and the @auto_str_repr decorator meaningfully reduces maintenance overhead and aligns well with the goal of automatically including new fields.
There are a few blocking issues to address before merge. Codecov is currently failing due to patch coverage being below the target; this appears to be mostly due to missing edge-case tests rather than untested core logic. Adding tests for boundary conditions, empty dataclasses, nested to_dict behavior, and decorator contract checks should bring coverage back above the threshold. Codacy also flags complexity and duplication in the formatting logic — both the mixin and decorator currently implement near-identical value formatting, and enum detection relies on MRO inspection. Extracting this into a shared helper and switching to isinstance(value, enum.Enum) would reduce complexity, eliminate duplication, and address those warnings cleanly.
Additionally, the PR currently has merge conflicts with main, including duplicated changelog entries, which need to be resolved to clear the merge detector.
If you need assistance or have any questions please reach out :)
|
I'd like to request opinions from @manishdait and @nadineloepfe before proceeding further with this |
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
tech0priyanshu
force-pushed
the
feat/auto-str
branch
from
571b702 to
e896596
Compare
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdsrc/hiero_sdk_python/tokens/custom_fee.pysrc/hiero_sdk_python/tokens/token_relationship.pysrc/hiero_sdk_python/tokens/token_update_transaction.pysrc/hiero_sdk_python/utils/dataclass_strings.pytests/unit/dataclass_strings_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/dataclass_strings_test.py
🧬 Code graph analysis (4)
src/hiero_sdk_python/tokens/token_update_transaction.py (1)
src/hiero_sdk_python/utils/dataclass_strings.py (1)
DataclassStringMixin(59-132)
src/hiero_sdk_python/tokens/custom_fee.py (2)
src/hiero_sdk_python/utils/dataclass_strings.py (1)
_format_value_helper(42-56)src/hiero_sdk_python/utils/subscription_handle.py (1)
join(30-35)
src/hiero_sdk_python/tokens/token_relationship.py (1)
src/hiero_sdk_python/utils/dataclass_strings.py (1)
DataclassStringMixin(59-132)
tests/unit/dataclass_strings_test.py (5)
src/hiero_sdk_python/utils/dataclass_strings.py (3)
DataclassStringMixin(59-132)auto_str_repr(135-186)to_dict(114-132)src/hiero_sdk_python/tokens/token_relationship.py (1)
TokenRelationship(22-120)src/hiero_sdk_python/tokens/token_id.py (1)
TokenId(21-180)src/hiero_sdk_python/account/account_id.py (1)
AccountId(21-198)src/hiero_sdk_python/tokens/custom_fixed_fee.py (1)
CustomFixedFee(22-300)
🪛 Ruff (0.14.10)
tests/unit/dataclass_strings_test.py
200-200: Possible hardcoded password assigned to argument: "token_name"
(S106)
src/hiero_sdk_python/utils/dataclass_strings.py
30-30: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
42-42: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
110-110: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
161-161: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.13)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (13)
CHANGELOG.md (1)
10-11: LGTM! Clear and accurate changelog entries.The changelog entries accurately document the dataclass-driven string representation refactoring and correctly reference issue #999. The formatting is clean with no markdown issues.
src/hiero_sdk_python/tokens/token_relationship.py (1)
19-22: LGTM! Clean migration to DataclassStringMixin.The migration to DataclassStringMixin is implemented correctly. The class remains a dataclass and now automatically generates string representations for all fields, eliminating the need for manual maintenance when fields are added or modified.
src/hiero_sdk_python/tokens/token_update_transaction.py (1)
26-50: LGTM! Consistent application of DataclassStringMixin.Both
TokenUpdateParamsandTokenUpdateKeysnow inherit fromDataclassStringMixin, providing automatic string representations. This is consistent with the approach used forTokenRelationshipand achieves the PR's goal of reducing maintenance burden.src/hiero_sdk_python/tokens/custom_fee.py (1)
155-179: LGTM! Appropriate string representation for ABC.The implementation correctly adds dynamic string representations to
CustomFee. Since this is an abstract base class (not a dataclass), it cannot inherit fromDataclassStringMixin. The approach of iterating through__dict__.items()and using the shared_format_value_helperis appropriate and maintains consistency with the dataclass-based implementations.The logic correctly:
- Skips private attributes (starting with
_)- Uses the shared formatting helper to avoid duplication
- Produces compact single-line output for ≤3 fields
- Uses readable multi-line output for >3 fields
src/hiero_sdk_python/utils/dataclass_strings.py (4)
30-56: LGTM! Clean and well-structured helper functions.The helper functions are well-implemented:
_is_enumuses the cleanerisinstance(value, enum.Enum)approach (addressing previous feedback)_format_bytesprovides appropriate truncation for long byte sequences_format_value_helpercentralizes formatting logic, ensuring consistency across the moduleThe shared
_format_value_helpersuccessfully eliminates code duplication between the mixin, decorator, andCustomFeeimplementations.
59-132: LGTM! Robust mixin implementation.
DataclassStringMixinis well-designed and implemented:
- Comprehensive documentation with clear usage example
- Proper runtime validation that the class is a dataclass
- Clean field introspection using
dataclasses.fields()- Appropriate formatting choice based on field count (compact for ≤3 fields, readable multi-line for >3)
- The
to_dict()method provides useful serialization support with proper handling of nested dataclassesThe implementation successfully achieves the PR objective of zero-maintenance string representations that automatically include new fields.
135-186: LGTM! Well-implemented decorator alternative.The
auto_str_reprdecorator provides a clean alternative to mixin inheritance:
- Proper validation that the target is a dataclass with a clear error message
- Consistent formatting logic with the mixin approach
- Correctly attaches
__str__and__repr__methods to the class- Well-documented with usage examples
This provides flexibility for users who prefer decorator-based composition over inheritance.
1-28: LGTM! Excellent module documentation.The module-level documentation is comprehensive and helpful:
- Clearly explains the purpose and benefits
- Provides concrete usage examples for both the mixin and decorator approaches
- Makes the value proposition clear: eliminating manual maintenance when fields are added
tests/unit/dataclass_strings_test.py (5)
1-52: LGTM! Well-structured test fixtures.The module docstring is clear, imports are correct, and the test fixtures (SampleEnum, SimpleDataclass, ComplexDataclass, DataclassWithEnum, DecoratedDataclass) provide good coverage for various scenarios. The fixtures appropriately test simple vs. complex dataclasses, optional fields, enums, and both mixin and decorator approaches.
55-145: LGTM! Comprehensive coverage of DataclassStringMixin.The tests thoroughly cover the mixin's behavior including single-line vs. multi-line formatting, None handling, string quoting, enum formatting, bytes handling with truncation, and the to_dict() method. All assertions are clear and deterministic.
147-169: LGTM! Decorator tests cover key functionality.The tests verify that the decorator correctly adds str/repr to dataclasses and properly raises TypeError when applied to non-dataclass types. Additional decorator tests (identity and return types) are appropriately covered in TestBoundaryConditions.
237-260: LGTM! Dynamic field inclusion test validates key benefit.The test correctly verifies that adding fields to a dataclass definition automatically includes them in the string representation without manual str updates. This addresses the core maintenance benefit described in issue #999.
Note: A past review comment suggested renaming to
test_dataclass_definition_changes_require_no_str_updates, but the current name and docstring are sufficiently clear.
262-361: Excellent! Comprehensive boundary and edge case coverage.This test class addresses multiple suggestions from past review comments and adds thorough edge case coverage:
✅ 3 vs. 4 field boundary for single-line/multi-line formatting
✅ Empty dataclass handling
✅ Nested dataclass to_dict conversion
✅ Decorator identity preservation (in-place modification)
✅ Return type verification
✅ Zero/False value formatting (protects against truthy/falsy bugs)
✅ Special character handling
✅ Collection field supportThe comprehensive coverage protects against regressions and validates edge cases that users might encounter.
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdtests/unit/dataclass_strings_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/dataclass_strings_test.py
🧬 Code graph analysis (1)
tests/unit/dataclass_strings_test.py (1)
src/hiero_sdk_python/utils/dataclass_strings.py (3)
DataclassStringMixin(59-132)auto_str_repr(135-186)to_dict(114-132)
🪛 Ruff (0.14.10)
tests/unit/dataclass_strings_test.py
205-205: Possible hardcoded password assigned to argument: "token_name"
(S106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.13)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (5)
tests/unit/dataclass_strings_test.py (5)
1-53: LGTM!The imports are valid and reference actual SDK modules. The test fixture classes are well-designed to test different scenarios: single-line vs multi-line output, enum formatting, and decorator behavior.
55-145: Good test coverage for DataclassStringMixin.The tests comprehensively cover the mixin's functionality: string formatting, repr behavior, multi-line output, None handling, string quoting, enum formatting, bytes formatting with truncation, and dictionary conversion.
Per coding guidelines, consider adding descriptive assertion messages for key assertions to aid debugging when tests fail. For example:
assert "SimpleDataclass(" in result, f"Expected class name prefix, got: {result}"However, the current assertions are clear and pytest's default output would provide sufficient context.
147-169: LGTM!Good test coverage for the decorator with proper error handling validation using
pytest.raiseswith thematchparameter.
172-255: Excellent integration tests with proper breaking change protection.The integration tests effectively:
- Use
hasattrchecks to protect against breaking changes (Priority 1)- Import from actual SDK module paths (Priority 4)
- Verify both inherited fields (
fee_collector_account_id,all_collectors_are_exempt) and class-specific fields (amount) forCustomFixedFeeThe static analysis hint about "hardcoded password" on line 205 (
token_name="Updated Token") is a false positive—this is a token property name, not a credential.
257-280: LGTM!The test effectively demonstrates the core value proposition of the mixin: new dataclass fields are automatically included in string representations without manual updates.
Signed-off-by: tech0priyanshu <priyanshuyadv101106@gmail.com>
|
@exploreriii @manishdait @aceppaluni @nadineloepfe please review |
|
Hi @tech0priyanshu i have to think about this more |
|
Sure @exploreriii I’m marking this as a draft for now. |
4 participants
Description:
Related issue(s):
Fixes #999
Notes for reviewer:
Checklist