-
Notifications
You must be signed in to change notification settings - Fork 185
chore: enhance exception type safety and context #1594
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
chore: enhance exception type safety and context #1594
Conversation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1594 +/- ##
==========================================
+ Coverage 92.89% 92.90% +0.01%
==========================================
Files 140 140
Lines 8767 8769 +2
==========================================
+ Hits 8144 8147 +3
+ Misses 623 622 -1 🚀 New features to boost your workflow:
|
WalkthroughAdds type annotations and return type hints to three exception classes, adds unit tests for those exceptions, introduces two new error example scripts, refactors an existing example to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 unit tests (beta)
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 |
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.
Pull request overview
This PR improves type safety and developer ergonomics around core exception types and their usage, and adds tests and documentation updates to validate and describe the behavior.
Changes:
- Added precise constructor and string/representation type hints for
PrecheckError,MaxAttemptsError, andReceiptStatusError, including forward-referenced types for transaction context. - Introduced unit tests exercising these exception types’ initialization, default message generation, and string/
reprbehavior. - Modernized the
examples/errors/receipt_status_error.pyexample to useClient.from_env()and the high-level package imports, and documented the change inCHANGELOG.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/hiero_sdk_python/exceptions.py |
Adds type hints to exception constructors and __str__/__repr__ methods, and uses TYPE_CHECKING imports to avoid runtime cycles while enabling structured error context types. |
tests/unit/exceptions_test.py |
New unit tests validate typed construction, default message generation, and string/repr output for the three exception classes. |
examples/errors/receipt_status_error.py |
Updates the error-handling example to use Client.from_env(), the package-level imports (Client, ResponseCode, TokenAssociateTransaction, TokenId), and to align with the stricter ReceiptStatusError typing. |
CHANGELOG.md |
Records the addition of type hints for the exception classes’ constructors and string methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Actionable comments posted: 2
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/errors/receipt_status_error.py (1)
52-58: Simplify redundantResponseCode()wrapping.Same as in
precheck_error.py:e.statusis already aResponseCodeenum, so wrapping it again is unnecessary.♻️ Suggested simplification
except ReceiptStatusError as e: print("\nCaught ReceiptStatusError!") - print(f"Status: {e.status} ({ResponseCode(e.status).name})") + print(f"Status: {e.status} ({e.status.name})") print(f"Transaction ID: {e.transaction_id}")
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.
Actionable comments posted: 1
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
Signed-off-by: Adityarya11 <[email protected]>
Signed-off-by: Adityarya11 <[email protected]>
Signed-off-by: Adityarya11 <[email protected]>
728ce48 to
717123c
Compare
aceppaluni
left a comment
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.
@Adityarya11
I looked into _Executable._execute, and at the moment MaxAttemptsError is always constructed with err_persistant, which is a formatted string (including in the grpc.RpcError path).
Given that current usage, I agree that keeping last_error typed as Optional[str] is reasonable for now and accurately reflects how the SDK behaves today.
If we later decide to propagate the underlying grpc.RpcError (or another concrete Exception) instead of a formatted message, we can revisit this and widen the type or refactor the execution path in a follow-up PR. For this change, keeping the scope minimal makes sense.
Yeah got it Thanks..👍 may be a GFI would make sense for that :) |
aceppaluni
left a comment
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.
@Adityarya11 This is great work, awesome job!
Description:
Type hints to exception classes (
PrecheckError,MaxAttemptsError,ReceiptStatusError) constructors and string methods.Related issue(s):
Fixes #1504
Notes for reviewer:
Tested the unit test and the example
Checklist