refactor: improve Request.from_dict to have fewer exceptions to the rule#726
refactor: improve Request.from_dict to have fewer exceptions to the rule#726
Request.from_dict to have fewer exceptions to the rule#726Conversation
|
@coderabbitai full review |
Actions performedFull review triggered. |
## Walkthrough
The recent updates involve modifications to the `CHANGELOG.md` file to document enhancements and fixes related to the `amm_info` feature in the `Request.from_dict` method, including improved error handling. Additionally, the `ABBREVIATIONS` dictionary in `xrpl/models/base_model.py` has been expanded with new entries for NFT-related terms. Changes in `xrpl/models/requests/request.py` include an updated import statement and a streamlined method for parsing request method names using the `ABBREVIATIONS` dictionary.
## Changes
| Files | Change Summary |
|-----------------------------------|---------------------------------------------------------------------------------------------------------------------|
| `CHANGELOG.md` | Updated to include new entries for `amm_info` support in `Request.from_dict`, improved error handling, and enhanced model validation. |
| `xrpl/models/base_model.py` | Expanded `ABBREVIATIONS` dictionary with new NFT-related terms and clarified comments without altering functionality. |
| `xrpl/models/requests/request.py` | Updated import statement to include `ABBREVIATIONS`; streamlined method name parsing logic using the `ABBREVIATIONS` dictionary. |
## Suggested reviewers
- ckeshava
> 🐰 In the changelog, we hop with glee,
> `amm_info` support is now a key!
> With abbreviations, our models grow,
> Parsing methods with a new flow.
> Validation's stronger, errors refined,
> In our rabbit world, progress aligned! 🐇✨📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🪛 LanguageToolCHANGELOG.md[uncategorized] ~13-~13: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE) ⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
11-11: Refine phrasing for clarity
The entry reads:- Added `MPTCurrency` support in `Issue` (rippled internal type)For consistency and readability, consider reordering and adding the article:
- Added `MPTCurrency` support in `Issue` (rippled internal type) + Added support for the `MPTCurrency` type in `Issue` (rippled internal type)
15-16: Improve wording and specificity
The current items:- Added support for `amm_info` to `Request.from_dict` - Improved erroring for `amm_info`Could be made clearer:
- Added support for `amm_info` to `Request.from_dict` - Improved erroring for `amm_info` + Added support for the `amm_info` RPC in `Request.from_dict` + Improved error handling for `amm_info` in `Request.from_dict`
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~13-~13: You might be missing the article “the” here.
Context: ... extracting sequence number. - Increase default maximum payload size for websocket clie...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.8)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
16-16: Improve phrasing for error handling descriptionThe term “erroring” is ambiguous. Consider rephrasing to “Improved error handling for
amm_info” for clarity:- - Improved erroring for `amm_info` + - Improved error handling for `amm_info`
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~13-~13: You might be missing the article “the” here.
Context: ... extracting sequence number. - Increase default maximum payload size for websocket clie...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
🔇 Additional comments (2)
CHANGELOG.md (2)
11-11: Consistent tense for changelog entriesGreat catch updating the phrasing to past-tense for MPTCurrency support. The entry is clear and aligns with other items.
15-15: Document external-facingamm_infosupportAdding
amm_infohere accurately reflects the new feature inRequest.from_dict. This is user-relevant and appropriately placed under “Fixed”.
3 participants
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.