refactor: improve Request.from_dict to have fewer exceptions to the rule
#726
Conversation
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThe recent updates involve modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (2)xrpl/models/base_model.py (2)
The comment clearly explains the purpose of the
The new NFT and NoRipple related abbreviations follow the established pattern and are properly ordered. Let's verify the usage of these new abbreviations in the codebase: ✅ Verification successfulNew abbreviations are actively used in the codebase The verification shows these abbreviations are well-integrated across the codebase:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for usage of new NFT and NoRipple related terms in the codebase
# Search for NFT related classes and fields
echo "Checking NFT related usage:"
rg -l "(?i)(NFT|NFToken)" --type py
# Search for NoRipple related classes and fields
echo -e "\nChecking NoRipple related usage:"
rg -l "(?i)NoRipple" --type py
# Search for potential JSON key conversions
echo -e "\nChecking potential JSON key conversions:"
rg -l "(?i)(nft|nftoken|noripple).*[\"']" --type py
Length of output: 3183 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai full review |
Actions performedFull review triggered. |
|
@coderabbitai full review |
Actions performedFull review triggered. |
|
@coderabbitai full review |
Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
tests/unit/models/requests/test_requests.py (1)
3-4: Consider avoiding the import of a protected memberIn line 4,
_DEFAULT_API_VERSIONis imported fromxrpl.models.requests.request. The leading underscore indicates that it's intended for internal use (a protected member). Importing protected members can lead to potential issues if the internal implementation changes.Suggestion:
- If possible, access
_DEFAULT_API_VERSIONthrough a public interface or define it in a constants module that is meant for external use.- If importing a protected member is necessary for testing purposes, consider adding a comment explaining why this import is required.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- snippets/bridge_transfer.py (1 hunks)
- tests/unit/models/requests/test_amm_info.py (2 hunks)
- tests/unit/models/requests/test_requests.py (1 hunks)
- xrpl/models/base_model.py (1 hunks)
- xrpl/models/requests/amm_info.py (2 hunks)
- xrpl/models/requests/request.py (3 hunks)
Additional comments not posted (19)
xrpl/models/requests/amm_info.py (1)
40-46: Excellent addition of the_get_errorsmethod to enhance validation logic!The new
_get_errorsmethod in theAMMInfoclass significantly improves the validation of theasset,asset2, andamm_accountfields. By ensuring thatassetandasset2are provided together and that they are mutually exclusive withamm_account, this method helps catch potential errors early in the request handling process.The specific error messages added in this method provide clear guidance to the caller about the expected input format, making it easier to diagnose and fix any issues.
Overall, this change enhances the robustness and reliability of the AMM information retrieval process by enforcing stricter input validation. Great work!
tests/unit/models/requests/test_amm_info.py (4)
29-33: LGTM!The
test_amounttest correctly validates that providing only theamm_accountparameter is a valid configuration for theAMMInforequest model.
35-41: LGTM!The
test_all_threetest correctly validates that providing all three parameters (amm_account,asset, andasset2) together is an invalid configuration for theAMMInforequest model and raises anXRPLModelException.
43-47: LGTM!The
test_only_assettest correctly validates that providing only theassetparameter is an invalid configuration for theAMMInforequest model and raises anXRPLModelException.
49-53: LGTM!The
test_only_one_asset2test correctly validates that providing only theasset2parameter is an invalid configuration for theAMMInforequest model and raises anXRPLModelException.snippets/bridge_transfer.py (1)
58-58: LGTM! The sleep delay should help improve code stability.The addition of the 0.5-second sleep delay after the account creation process is a good precautionary measure to mitigate potential flakiness or timing issues that could arise if the balance retrieval is attempted immediately after the account creation.
This delay provides a buffer time for the account creation to fully propagate before attempting to retrieve the initial balance, thereby improving the stability of the code execution.
xrpl/models/requests/request.py (3)
15-15: LGTM!The import of
ABBREVIATIONSfromxrpl.models.base_modellooks good.
134-135: LGTM!The TODO comment about supporting the "command" parameter and proper JSON RPC format is informative and aligns with the existing functionality in
GenericRequest. It does not introduce any functional changes at the moment.
173-178: Excellent refactoring!The modifications to the
get_methodclass method significantly improve the parsing of method names and the resolution of the correct request class. The key benefits include:
- The use of the
ABBREVIATIONSmapping provides a more dynamic and maintainable approach compared to the previous series of conditional checks.- The parsing logic handles both abbreviated and non-abbreviated method names by capitalizing words if no abbreviation exists, making it more flexible.
- Checking the parsed name against
xrpl.models.requests.__all__allows for a more extensible resolution of the correct request class, addressing the concern of missingamm_infoin the previous implementation.These changes align perfectly with the PR objectives of reducing exceptions and handling
amm_infoby generalizing the parsing logic. The refactoring effectively addresses the maintainability and extensibility issues raised in the past review comments.xrpl/models/base_model.py (4)
37-38: LGTM!The comment provides clear and useful context about the purpose of the
ABBREVIATIONSdictionary.
43-43: LGTM!The addition of the
"nft": "NFT"key-value pair to theABBREVIATIONSdictionary is relevant and consistent with the purpose of the dictionary.
45-45: LGTM!The addition of the
"nfts": "NFTs"key-value pair to theABBREVIATIONSdictionary is relevant and consistent with the purpose of the dictionary.
46-46: LGTM!The addition of the
"noripple": "NoRipple"key-value pair to theABBREVIATIONSdictionary is relevant and consistent with the purpose of the dictionary.CHANGELOG.md (2)
24-24: LGTM!Adding support for
amm_infotoRequest.from_dictaligns with the PR objectives and improves the functionality by fixing an overlooked exception.
25-25: Verify the improved error handling foramm_info.Enhancing the error handling for
amm_infoaligns with the PR objectives and improves the robustness of the code.To verify the improved error handling, consider running the following script:
Verification successful
Verification of improved error handling for
amm_infocomplete.The implementation of
AMMInfoinxrpl/models/requests/amm_info.pyshows improved error handling with specific checks for parameter combinations. The_get_errorsmethod ensures that:
- Both
assetandasset2fields are provided together or neither is provided.- The
amm_accountfield is not provided along withassetandasset2fields.The unit tests in
tests/unit/models/requests/test_amm_info.pycover various scenarios, including:
- Valid parameter combinations
- Invalid combinations that should raise exceptions
These tests ensure that the error handling works as expected for different input scenarios.
While no specific integration tests for
amm_infoerror handling were found, the unit tests provide good coverage of the error cases.The implementation and tests support the changelog's claim of "Improved erroring for
amm_info" in the 3.0.0 release.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the improved error handling for `amm_info`. # Test 1: Search for the error handling code. Expect: Relevant error handling code for `amm_info`. rg --type python -A 5 $'amm_info.*Error' # Test 2: Search for test cases that cover the error scenarios. Expect: Relevant test cases. rg --type python -A 10 $'def test_.*amm_info.*error'Length of output: 157
Script:
#!/bin/bash # Description: Verify the improved error handling for `amm_info` and check repository structure. # Check for Python files echo "Checking for Python files:" fd -e py # Search for amm_info without file type restrictions echo -e "\nSearching for amm_info:" rg -i "amm_info" -A 5 # Look for test files related to amm_info echo -e "\nSearching for test files related to amm_info:" rg -i "test.*amm_info" # Check CHANGELOG.md for context echo -e "\nChecking CHANGELOG.md for context:" cat CHANGELOG.mdLength of output: 80367
Script:
#!/bin/bash # Description: Examine AMMInfo implementation and tests # Check AMMInfo implementation echo "AMMInfo implementation:" cat xrpl/models/requests/amm_info.py # Check unit tests for AMMInfo echo -e "\nAMMInfo unit tests:" cat tests/unit/models/requests/test_amm_info.py # Look for integration tests related to amm_info error handling echo -e "\nIntegration tests for amm_info error handling:" rg -i "test.*amm_info.*error" tests/integrationLength of output: 3242
tests/unit/models/requests/test_requests.py (4)
20-31: Test forfrom_dictmethod ofAccountTxappears correctThe test
test_from_dictcorrectly verifies thatRequest.from_dict(req)instantiates anAccountTxobject with the expected default values. Theexpecteddictionary includes default parameters (binary,forward, andapi_version), and the assertion confirms thatobj.to_dict()matchesexpected.
32-36: Test for handling missingmethodkey is appropriateThe test
test_from_dict_no_methodcorrectly checks that anXRPLModelExceptionis raised when the input dictionary lacks the requiredmethodkey. This ensures that thefrom_dictmethod validates input as expected.
37-41: Test for mismatchedmethodinfrom_dictis validIn
test_from_dict_wrong_method, the test ensures thatFee.from_dict(req)raises anXRPLModelExceptionwhen themethoddoes not match the expected value for theFeeclass. This is an important validation to prevent incorrect instantiation.
93-101: Test forGenericRequestinstantiation is accurateThe
test_from_dict_generic_requestmethod correctly verifies that aGenericRequestis instantiated when usingRequest.from_dict(req)with a method not explicitly handled by other request classes. This ensures that generic requests are appropriately managed.
| req = GenericRequest(command=command).to_dict() | ||
| expected = {**req, "api_version": _DEFAULT_API_VERSION} | ||
| self.assertDictEqual(req, expected) |
There was a problem hiding this comment.
Review the correctness of the test assertion
In this test, req is converted to a dictionary, and then expected is created by merging req with {"api_version": _DEFAULT_API_VERSION}:
req = GenericRequest(command=command).to_dict()
expected = {**req, "api_version": _DEFAULT_API_VERSION}
self.assertDictEqual(req, expected)However, since expected adds api_version to req, and then you're asserting that req and expected are equal, this assertion will fail unless api_version is already included in req. This might indicate an issue with the test logic.
Suggestion:
-
Verify whether
reqalready includesapi_version. If it does, adding it again inexpectedis redundant. -
If
reqdoes not includeapi_version, the assertion will fail. Consider modifying the test to includeapi_versioninreqbefore the assertion, or adjust the expected dictionary accordingly. -
Alternatively, if the goal is to ensure that
api_versionis present inreq, you might want to assert that directly:self.assertIn("api_version", req)
There was a problem hiding this comment.
api_version is added in the to_dict function.
There was a problem hiding this comment.
@mvadari, thank you for clarifying. Since api_version is added in the to_dict function, the test assertion is correct. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: mvadari
PR: XRPLF/xrpl-py#726
File: tests/unit/models/requests/test_requests.py:16-18
Timestamp: 2024-11-01T20:40:21.201Z
Learning: In the xrpl-py codebase, the `to_dict` method of classes inheriting from `Request` automatically includes the `api_version` key in the resulting dictionary. When reviewing tests that use `to_dict()`, expect `api_version` to be present in the output.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
mvadari
changed the title
Request.from_dict to have fewer exceptionsRequest.from_dict to have fewer exceptions to the rule
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
1-24: Consider improving the changelog format.The changelog follows the Keep a Changelog format but could be improved:
- The "Unreleased" section should be at the top (which it is)
- Each change should start with a verb in the imperative mood (e.g., "Add" instead of "Added", "Remove" instead of "Remove")
- Breaking changes should be marked with "!" after the type (e.g., "### Changed!")
Consider applying these format improvements in future entries. Example:
## [[Unreleased]] ### Added -- Add `include_deleted` to ledger_entry request +- Add `include_deleted` to ledger_entry request -### BREAKING CHANGE: +### Changed! - Remove Python 3.7 support to fix dependency installation and use 3.8 as new default.
Future release date detected in CHANGELOG.md
The version 3.0.0 is listed with a future date (2024-07-16) which is incorrect for a released version. The version number is also present in
pyproject.toml, indicating this is indeed the current version, but it should have a past release date.
CHANGELOG.md: Update the release date for version 3.0.0 to reflect the actual release datepyproject.tomlconfirms version 3.0.0 is the current version🔗 Analysis chain
Line range hint
1-24: Verify version numbers and dates.The changelog shows version 3.0.0 as the latest released version, dated 2024-07-16. This appears to be a future date.
Let's verify the version numbers and dates:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the version numbers and dates are consistent with the repository # Test 1: Check if there are any other files that mention the version number echo "Checking for version numbers in other files..." rg -i "version.*3\.0\.0|3\.0\.0.*version" # Test 2: Check if the date is mentioned elsewhere echo "Checking for the future date..." rg "2024-07-16"Length of output: 305
| - Added support for `amm_info` to `Request.from_dict` | ||
| - Improved erroring for `amm_info` |
There was a problem hiding this comment.
Add missing changelog entries for the main PR changes.
While the additions for amm_info support are documented, the changelog is missing entries for the following significant changes mentioned in the PR objectives:
- Refactoring of
Request.from_dictto reduce exceptions - Addition of tests for
Request.from_dict
Consider adding these under appropriate sections (e.g., "Changed" and "Added" respectively).
Apply this diff to add the missing entries:
### Fixed
- Allow empty strings for the purpose of removing fields in DIDSet transaction
- Added support for `amm_info` to `Request.from_dict`
- Improved erroring for `amm_info`
+
+### Changed
+- Refactored `Request.from_dict` to reduce the number of exceptions
+
+### Added
+- Added test coverage for `Request.from_dict` 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.
| - Added support for `amm_info` to `Request.from_dict` | |
| - Improved erroring for `amm_info` | |
| - Added support for `amm_info` to `Request.from_dict` | |
| - Improved erroring for `amm_info` | |
| ### Changed | |
| - Refactored `Request.from_dict` to reduce the number of exceptions | |
| ### Added | |
| - Added test coverage for `Request.from_dict` method |
High Level Overview of Change
This PR:
Request.from_dictto have fewer exceptions to the general processing, to be more future-proofRequest.from_dict(there were previously no tests)Request.from_dictforamm_info(an exception that was forgotten)amm_infoRPCContext of Change
I noticed the number of exceptions to the general case (i.e. extra
ifstatements) in theRequest.from_dictmethod the other day and wanted to fix it.Type of Change
Did you update CHANGELOG.md?
Test Plan
Added tests for untested/undertested functions, and all the new code.