-
Notifications
You must be signed in to change notification settings - Fork 147
docs: enhance FeeAssessmentMethod enum docstring #1393
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
docs: enhance FeeAssessmentMethod enum docstring #1393
Conversation
- Add comprehensive docstring explaining inclusive vs exclusive fee assessment - Clarify when each method should be used and their payment implications - Update changelog entry Fixes #1391 Signed-off-by: cheese-cakee <[email protected]>
- Add comprehensive docstring explaining fungible vs non-fungible tokens - Provide use cases and examples for each token type - Update changelog entry Fixes #1392 Signed-off-by: cheese-cakee <[email protected]>
📝 WalkthroughWalkthroughAdds a comprehensive docstring to the FeeAssessmentMethod enum and introduces a new public enum member Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdsrc/hiero_sdk_python/tokens/fee_assessment_method.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/hiero_sdk_python/tokens/fee_assessment_method.py
7-7: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
10-10: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
src/hiero_sdk_python/tokens/fee_assessment_method.py (1)
18-19: The EXCLUSIVE enum member is properly integrated and tested.Verification confirms that
FeeAssessmentMethod.EXCLUSIVEis already used in unit tests (tests/unit/custom_fee_test.py, lines 117 and 130) with proper assertions verifying the enum value. The feature is documented in the docstring (lines 10-12), supports round-trip proto serialization/deserialization (line 171 ofcustom_fractional_fee.py), and integrates with the existingCustomFractionalFeeclass. No assumptions thatFeeAssessmentMethodcontains onlyINCLUSIVEwere found.
CHANGELOG.md
Outdated
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
| - Added comprehensive docstring to `FeeAssessmentMethod` enum explaining inclusive vs exclusive fee assessment methods. (#1391) |
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.
Changelog entry is incomplete—missing the EXCLUSIVE enum member addition.
The changelog entry only mentions the docstring addition, but the code changes also introduce a new EXCLUSIVE = 1 enum member. This is a functional change to the public API that should be documented in the changelog.
📝 Suggested changelog update
-- Added comprehensive docstring to `FeeAssessmentMethod` enum explaining inclusive vs exclusive fee assessment methods. (#1391)
+- Added comprehensive docstring to `FeeAssessmentMethod` enum explaining inclusive vs exclusive fee assessment methods. Added `EXCLUSIVE = 1` enum member to support fee charging in addition to the transferred amount. (#1391)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Added comprehensive docstring to `FeeAssessmentMethod` enum explaining inclusive vs exclusive fee assessment methods. (#1391) | |
| - Added comprehensive docstring to `FeeAssessmentMethod` enum explaining inclusive vs exclusive fee assessment methods. Added `EXCLUSIVE = 1` enum member to support fee charging in addition to the transferred amount. (#1391) |
Fix RUF002 linting error by replacing – with - in docstring to comply with code style guidelines. Signed-off-by: cheese-cakee <[email protected]>
Fix RUF002 linting error by replacing – with - in docstring to comply with code style guidelines. Signed-off-by: cheese-cakee <[email protected]>
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/hiero_sdk_python/tokens/fee_assessment_method.py
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
CHANGELOG.md
Outdated
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
| - Added comprehensive docstring to `FeeAssessmentMethod` enum explaining inclusive vs exclusive fee assessment methods. (#1391) |
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.
Please update the CHANGELOG.md to mention the new enum member, not just the documentation change.
exploreriii
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.
Hi @cheese-cakee
I think this is a really nice idea, your docstring is informative and not too long.
To push this dosctring to the next level, let's please use google-style docstrings
https://realpython.com/how-to-write-docstrings-in-python/
something very similar would be like this:
class FeeAssessmentMethod(Enum):
"""Fee assessment method for custom token fees.
Determines whether custom fees are deducted from the transferred amount
or charged separately.
Attributes:
INCLUSIVE: Fee is
"""
INCLUSIVE = 0
EXCLUSIVE = 1| Represents the fee assessment method for custom fees. | ||
| Fee assessment method for custom token fees: | ||
| • INCLUSIVE - Fee is deducted from the transferred amount. |
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.
The indendation for inclusive and exclusive need to match please
| Represents the fee assessment method for custom fees. | ||
| Fee assessment method for custom token fees: | ||
| • INCLUSIVE - Fee is deducted from the transferred amount. |
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.
These should be inside an attributes section
Attributes:
| The recipient receives the full transferred amount, and the payer | ||
| pays the fee on top of that. | ||
| This determines whether custom fees are taken from the transaction amount |
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.
The definitions should be above the attributes
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
…ples - Add comprehensive Google-style docstring with Attributes, Examples, Args, Returns sections - Include practical example usage for better developer understanding - Maintain existing functionality while improving documentation quality Addresses maintainer feedback for better docstring format Signed-off-by: cheese-cakee <[email protected]>
- Add comprehensive Google-style docstring with Attributes, Examples, Args, Returns sections - Include practical example usage for better developer understanding - Maintain existing functionality while improving documentation quality Addresses maintainer feedback for better docstring format Signed-off-by: cheese-cakee <[email protected]>
- Both PRs are up to date with latest changes - No actual merge conflicts detected - Documentation improvements implemented as requested by maintainer
Signed-off-by: cheese-cakee <[email protected]>
…ocstring improvements - Add changelog entries for PR #1393 and #1394 - Document enhanced docstrings with Google-style format and examples - Reflects proper contributor acknowledgments Prepares changelog for proper documentation of recent enhancements Signed-off-by: cheese-cakee <[email protected]>
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdsrc/hiero_sdk_python/tokens/fee_assessment_method.py
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/hiero_sdk_python/tokens/fee_assessment_method.py (1)
29-30: Inconsistency detected between AI summary and actual changes.The AI-generated summary claims this PR "introduces a new public enum member
EXCLUSIVE = 1", but lines 29-30 show no change markers (~). Only the docstring (lines 4-26) was modified. Both enum members appear to have existed prior to this PR.CHANGELOG.md (1)
1-1: LGTM!Normalizing the CHANGELOG header to standard ASCII markdown format improves consistency.
| Args: | ||
| value (int): The numeric value representing the fee assessment method. | ||
| Returns: | ||
| FeeAssessmentMethod: The enum instance for the specified method. |
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.
Remove Args and Returns sections from Enum docstring.
The Args and Returns sections are inappropriate for an Enum class docstring. Python Enums are accessed as FeeAssessmentMethod.INCLUSIVE or constructed via FeeAssessmentMethod(0), not called with explicit arguments as the docstring suggests. These sections will confuse users about how to use the enum.
📝 Suggested fix
Remove the Args and Returns sections entirely:
Example:
>>> # Using inclusive fee assessment
>>> assessment = FeeAssessmentMethod.INCLUSIVE
>>> print(f"Fee type: {assessment}")
Fee type: FeeAssessmentMethod.INCLUSIVE
-
- Args:
- value (int): The numeric value representing the fee assessment method.
-
- Returns:
- FeeAssessmentMethod: The enum instance for the specified method.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Args: | |
| value (int): The numeric value representing the fee assessment method. | |
| Returns: | |
| FeeAssessmentMethod: The enum instance for the specified method. | |
| Example: | |
| >>> # Using inclusive fee assessment | |
| >>> assessment = FeeAssessmentMethod.INCLUSIVE | |
| >>> print(f"Fee type: {assessment}") | |
| Fee type: FeeAssessmentMethod.INCLUSIVE | |
| """ |
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.
yes we can delete them, args and returns are helpful for functions only
- Force GitHub to update PR status with latest changes
This comment was marked as resolved.
This comment was marked as resolved.
- Add comprehensive docstring explaining fungible vs non-fungible tokens - Provide use cases and examples for each token type - Update changelog entry Fixes #1392 Signed-off-by: cheese-cakee <[email protected]>
Fix RUF002 linting error by replacing – with - in docstring to comply with code style guidelines. Signed-off-by: cheese-cakee <[email protected]>
- Add comprehensive Google-style docstring with Attributes, Examples, Args, Returns sections - Include practical example usage for better developer understanding - Maintain existing functionality while improving documentation quality Addresses maintainer feedback for better docstring format Signed-off-by: cheese-cakee <[email protected]>
- Both PRs are up to date with latest changes - No actual merge conflicts detected - Documentation improvements implemented as requested by maintainer
- Removed Args/Returns sections from enum classes as they don't accept parameters - Fixed indentation to follow Google-style docstring format - Maintained comprehensive Examples and Attributes sections
exploreriii
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.
This fee assessment PR should not include the token type commit
https://github.com/hiero-ledger/hiero-sdk-python/pull/1393/changes/9ef922c99cd8b94e2dae6da97ec6003f89b58e71..dc7d909edf3169f0e712a165665ded99d5e5402a
please
|
Hi @cheese-cakee please try again on an updated fork and new branch |
Fixes #1391