fix: better formatting handling in definitions script #879
Conversation
WalkthroughBroadened multiple parser regexes to accept optional spaces/newlines between tokens and updated binary codec definitions JSON by adding Int32/Int64 types and two new fields, removing one transaction result key, and reordering some transaction type entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Tool as generate_definitions.py
participant Spec as Source spec/text
participant Parser as Regex parser
participant JSON as definitions.json
Note over Tool,Spec: Read input spec or source lines
Tool->>Parser: Apply relaxed regexes (STYPE, TYPED_SFIELD, LEDGER_ENTRY, TER, TRANSACTION)
alt Match
Parser-->>Tool: Extracted tokens/groups
Tool->>JSON: Emit/augment types and fields (Int32, Int64, MutableFlags, DummyInt32)
JSON-->>Tool: Write updated definitions.json
else No match
Parser-->>Tool: Skip or warn
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (9)
🔇 Additional comments (4)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/generate_definitions.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
mvadari
changed the title
| "Int32": 10, | ||
| "Int64": 11, |
There was a problem hiding this comment.
I got this comment from coderabbit when I added these two lines in my PR. Is the comment valid and should we remove these two lines?
There was a problem hiding this comment.
It's not the only SType that isn't included in the codec, if you take a look at the full list. It doesn't matter as long as there are no fields that use the SType (which there aren't).
High Level Overview of Change
Fix the definitions generation script so that it better handles formatting changes, like adding new lines
Context of Change
Some newlines were introduced in
transactions.macro, which weren't properly captured by the regexType of Change
Did you update CHANGELOG.md?
Test Plan
Works locally.