Implemented underscore syntax in numbers #29
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces support for numeric literals with underscores in the Numscript language. The grammar definition in Changes
Sequence DiagramsequenceDiagram
participant Lexer
participant Parser
participant Test
Lexer->>Parser: Tokenize number with underscores
Parser->>Parser: Remove underscores
Parser->>Parser: Convert to integer
Test->>Parser: Verify parsing
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
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
CodeRabbit Configuration File (
|
| @@ -43,6 +43,14 @@ func TestVariable(t *testing.T) { | |||
| snaps.MatchSnapshot(t, p.Value) | |||
| } | |||
|
|
|||
| func TestNumberSyntaxUnderscore(t *testing.T) { | |||
| p := parser.Parse(` | |||
| set_tx_meta("k1", 1_2_34_567) | |||
There was a problem hiding this comment.
Here's the syntax
| Start: parser.Position{Character:18, Line:1}, | ||
| End: parser.Position{Character:28, Line:1}, | ||
| }, | ||
| Number: 1234567, |
There was a problem hiding this comment.
Here's the resulting syntax tree
![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
🧹 Nitpick comments (1)
internal/parser/parser_test.go (1)
46-52: Augment test coverage for underscore usage.The new test verifying “1_2_34_567” is a good starting point. Consider adding more edge-case tests (e.g., numbers with trailing underscores, multiple consecutive underscores, negative numbers with underscores) to ensure robust parsing of underscore-formatted numeric literals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (5)
Numscript.g4(1 hunks)internal/parser/antlr/NumscriptLexer.interp(1 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/parser.go(1 hunks)internal/parser/parser_test.go(1 hunks)
🔇 Additional comments (4)
internal/parser/antlr/numscript_lexer.go (1)
Line range hint 69-218: Auto-generated lexer updates for underscore syntax support.
These changes in the serialized ATN (from 326 to 328) reflect the updated grammar allowing underscores in numeric literals. Because this file is ANTLR-generated, no manual intervention is needed here. Just ensure that all underscore-related numeric cases (e.g., trailing underscores, multiple underscores, negative values) are covered by tests.
Numscript.g4 (1)
40-40: 🛠️ Refactor suggestion
Consider restricting underscores usage further.
This grammar rule “NUMBER: MINUS? [0-9][0-9_];” now allows trailing and consecutive underscores (e.g., "123_" or "1__2"). If that’s not intended, consider tightening the pattern (e.g., requiring underscores only between digits like "[0-9]+(_[0-9]+)") or adding validation logic post-parse to reject malformed numeric literals.
internal/parser/antlr/NumscriptLexer.interp (1)
128-128: Auto-generated lexer interpretation changes.
This shift from “326” to “328” confirms alignment with the updated grammar for underscores in numeric literals. No manual revision is required here.
internal/parser/parser.go (1)
588-588: LGTM! Basic underscore support implemented.
The implementation correctly removes underscores from number literals before parsing, enabling support for more readable number formats like 1_000_000.
| @@ -585,6 +585,7 @@ func parseSendStatement(statementCtx *parser.SendStatementContext) *SendStatemen | |||
|
|
|||
| func parseNumberLiteral(numNode antlr.TerminalNode) *NumberLiteral { | |||
| amtStr := numNode.GetText() | |||
| amtStr = strings.ReplaceAll(amtStr, "_", "") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating underscore placement.
The current implementation accepts any underscore placement, including invalid patterns like __123, _123, 123_, or 1__2. Consider adding validation to ensure underscores follow common programming language conventions:
- No leading or trailing underscores
- No consecutive underscores
- Only between digits
Here's a suggested implementation using regex validation:
func parseNumberLiteral(numNode antlr.TerminalNode) *NumberLiteral {
amtStr := numNode.GetText()
+ // Validate underscore placement: only single underscores between digits
+ if strings.Contains(amtStr, "_") {
+ if matched, _ := regexp.MatchString(`^-?\d+(_\d+)*$`, amtStr); !matched {
+ panic("Invalid number format: " + amtStr + ". Underscores must be between digits.")
+ }
+ }
amtStr = strings.ReplaceAll(amtStr, "_", "")Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
+ Coverage 63.24% 63.27% +0.03%
==========================================
Files 29 29
Lines 6456 6462 +6
==========================================
+ Hits 4083 4089 +6
Misses 2191 2191
Partials 182 182 ☔ View full report in Codecov by Sentry. |
No description provided.