fix: add conditional check for PermissionValue definition
#3018
Conversation
khancode
requested review from
ckniffen,
mvadari,
achowdhry-ripple and
Patel-Raj11
|
""" WalkthroughA conditional check was added to ensure the existence of the Changes
Possibly related issues
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ 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. 🪧 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
|
|
Can we add a test here that removes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ripple-binary-codec/test/definitions.test.ts (1)
168-172: Consider adding a guard for edge case scenarios.While the current implementation is correct, consider adding a guard to handle the edge case where PermissionValue might not exist in the original definitions.
// Remove PermissionValue from definitions if (definitions.FIELDS) { + const originalPermissionValue = definitions.FIELDS.find( + (fieldTuple: [string, object]) => fieldTuple[0] === 'PermissionValue' + ) + definitions.FIELDS = definitions.FIELDS.filter( (fieldTuple: [string, object]) => fieldTuple[0] !== 'PermissionValue', ) + + // Only verify removal if PermissionValue was originally present + if (originalPermissionValue) { + const expectedFieldsLength = originalFieldsLength - 1 + expect(definitions.FIELDS.length).toBe(expectedFieldsLength) + } } - - // Verify it was deleted - const expectedFieldsLength = originalFieldsLength - 1 - expect(definitions.FIELDS.length).toBe(expectedFieldsLength)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ripple-binary-codec/test/definitions.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: Analyze (javascript)
- GitHub Check: build-and-lint (22.x)
🔇 Additional comments (1)
packages/ripple-binary-codec/test/definitions.test.ts (1)
161-188: LGTM! Well-implemented test case addressing the PR requirements.This test effectively validates the conditional check fix for
PermissionValuedefinition that was mentioned in the PR objectives. The implementation follows the established patterns in the test file and properly addresses the concern raised by Patel-Raj11 about adding test coverage.The test logic is sound:
- Creates a proper deep copy of definitions
- Correctly removes the PermissionValue field using array filtering
- Verifies the removal was successful
- Tests that encoding/decoding still works without errors
khancode
force-pushed
the
permission-value-check
branch
from
2f53699 to
cd8f47f
Compare
High Level Overview of Change
Adds conditional check for
PermissionValueso custom definitions (based on previous v2.x versions) don't break.Context of Change
Bithomp reported this issue.
Type of Change
Did you update HISTORY.md?
Test Plan
Adds unit test to delete
PermissionValuefrom defintions.json and verified it doesn't throw an error anymore.