Experimental/oneof combinator #31
Conversation
|
Warning Rate limit exceeded@ascandone has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Parser
participant AST
participant Interpreter
Parser->>Parser: Encounter 'oneof' source
Parser->>AST: Create SourceOneof
AST-->>Parser: Return grouped sources
Parser->>Interpreter: Pass parsed sources
Interpreter->>Interpreter: Process sources
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
- Coverage 63.27% 63.25% -0.03%
==========================================
Files 29 29
Lines 6462 6766 +304
==========================================
+ Hits 4089 4280 +191
- Misses 2191 2289 +98
- Partials 182 197 +15 ☔ View full report in Codecov by Sentry. |
![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 (5)
internal/interpreter/interpreter.go (1)
984-991: Use pointer receiver forcheckFeatureFlagmethodFor consistency with other methods on
programStateand to avoid unnecessary copying of the struct, consider changing the receiver of thecheckFeatureFlagmethod to a pointer receiver*programState.Apply this diff:
-func (s programState) checkFeatureFlag(flag string) InterpreterError { +func (s *programState) checkFeatureFlag(flag string) InterpreterError {Numscript.g4 (1)
87-87: LGTM! Consider adding documentation.The new
oneofsource rule is well-structured and follows the grammar's conventions. Consider adding a comment to document the purpose and behavior of this new construct.Add a comment above the rule to explain its purpose:
+// oneof allows grouping multiple sources, where any one of them can be used as the source | 'oneof' LBRACE source* RBRACE # srcOneofinternal/parser/parser_test.go (1)
84-91: Enhance test coverage with additional test cases.While the basic test case is good, consider adding tests for:
- Empty
oneofblock- Nested
oneofblocks- Error cases (e.g., invalid source within
oneof)Example test cases:
func TestOneofSourceEdgeCases(t *testing.T) { tests := []struct { name string script string wantErrs bool }{ { name: "empty oneof block", script: `send $amt ( source = oneof { } destination = @d )`, wantErrs: true, }, { name: "nested oneof blocks", script: `send $amt ( source = oneof { @s1 oneof { @s2 @s3 } } destination = @d )`, wantErrs: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { p := parser.Parse(tt.script) if tt.wantErrs { assert.NotEmpty(t, p.Errors) } else { assert.Empty(t, p.Errors) snaps.MatchSnapshot(t, p.Value) } }) } }internal/parser/parser.go (1)
173-182: Consider adding validation for empty sources.The implementation looks good and follows the pattern used for
SrcInorderContext. However, consider adding validation to ensure that at least one source is provided in theoneofblock, as an emptyoneofblock might not be semantically valid.Consider this enhancement:
case *parser.SrcOneofContext: var sources []Source for _, sourceCtx := range sourceCtx.AllSource() { sources = append(sources, parseSource(sourceCtx)) } + if len(sources) == 0 { + // Add to ErrorListener or handle empty oneof block + listener.Errors = append(listener.Errors, ParserError{ + Range: range_, + Msg: "oneof block must contain at least one source", + }) + } return &SourceOneof{ Range: range_, Sources: sources, }internal/parser/antlr/numscript_lexer.go (1)
Line range hint
1-1: Reminder: This is an auto-generated file.Changes to this file should be made through the
Numscript.g4grammar file, as any manual edits here will be overwritten when the lexer is regenerated.
📜 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 (13)
Numscript.g4(1 hunks)internal/interpreter/interpreter.go(4 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/Numscript.tokens(1 hunks)internal/parser/antlr/NumscriptLexer.interp(4 hunks)internal/parser/antlr/NumscriptLexer.tokens(1 hunks)internal/parser/antlr/numscript_base_listener.go(1 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/antlr/numscript_listener.go(2 hunks)internal/parser/antlr/numscript_parser.go(23 hunks)internal/parser/ast.go(2 hunks)internal/parser/parser.go(1 hunks)internal/parser/parser_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/parser/antlr/Numscript.tokens
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/parser/antlr/numscript_base_listener.go
[warning] 185-185: internal/parser/antlr/numscript_base_listener.go#L185
Added line #L185 was not covered by tests
[warning] 188-188: internal/parser/antlr/numscript_base_listener.go#L188
Added line #L188 was not covered by tests
internal/parser/ast.go
[warning] 90-90: internal/parser/ast.go#L90
Added line #L90 was not covered by tests
internal/parser/antlr/numscript_parser.go
[warning] 2786-2787: internal/parser/antlr/numscript_parser.go#L2786-L2787
Added lines #L2786 - L2787 were not covered by tests
[warning] 2790-2791: internal/parser/antlr/numscript_parser.go#L2790-L2791
Added lines #L2790 - L2791 were not covered by tests
[warning] 2794-2795: internal/parser/antlr/numscript_parser.go#L2794-L2795
Added lines #L2794 - L2795 were not covered by tests
[warning] 2819-2826: internal/parser/antlr/numscript_parser.go#L2819-L2826
Added lines #L2819 - L2826 were not covered by tests
[warning] 2828-2828: internal/parser/antlr/numscript_parser.go#L2828
Added line #L2828 was not covered by tests
[warning] 2832-2834: internal/parser/antlr/numscript_parser.go#L2832-L2834
Added lines #L2832 - L2834 were not covered by tests
[warning] 2836-2836: internal/parser/antlr/numscript_parser.go#L2836
Added line #L2836 was not covered by tests
[warning] 2839-2842: internal/parser/antlr/numscript_parser.go#L2839-L2842
Added lines #L2839 - L2842 were not covered by tests
[warning] 2845-2848: internal/parser/antlr/numscript_parser.go#L2845-L2848
Added lines #L2845 - L2848 were not covered by tests
[warning] 3393-3394: internal/parser/antlr/numscript_parser.go#L3393-L3394
Added lines #L3393 - L3394 were not covered by tests
[warning] 3400-3400: internal/parser/antlr/numscript_parser.go#L3400
Added line #L3400 was not covered by tests
[warning] 3413-3413: internal/parser/antlr/numscript_parser.go#L3413
Added line #L3413 was not covered by tests
[warning] 3421-3422: internal/parser/antlr/numscript_parser.go#L3421-L3422
Added lines #L3421 - L3422 were not covered by tests
[warning] 3433-3434: internal/parser/antlr/numscript_parser.go#L3433-L3434
Added lines #L3433 - L3434 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
internal/parser/ast.go (2)
106-109: LGTM!The
SourceOneoftype is well-structured and consistent with other source types in the codebase.
90-90: Add test coverage for thesource()method.The
source()method forSourceOneofis not covered by tests.Run the following script to verify test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 90-90: internal/parser/ast.go#L90
Added line #L90 was not covered by testsinternal/parser/antlr/Numscript.interp (1)
4-4: Skip review of auto-generated file.This file is auto-generated by ANTLR and doesn't require manual review.
Also applies to: 45-45, 106-106
internal/parser/antlr/numscript_listener.go (1)
89-91: LGTM! The new listener methods are well-integrated.The added
EnterSrcOneofandExitSrcOneofmethods follow the established pattern and are properly documented.Also applies to: 212-214
internal/parser/antlr/NumscriptLexer.interp (1)
4-4: LGTM! The lexer changes are consistent.The new token literal 'oneof' and associated rules are properly integrated into the lexer's token definitions.
Also applies to: 45-45, 85-85, 131-131
internal/parser/antlr/numscript_base_listener.go (1)
184-189: LGTM! Base listener implementation follows the pattern.The empty implementations for
EnterSrcOneofandExitSrcOneofare correct. The static analysis warning about test coverage can be safely ignored as this is generated code with intentionally empty implementations.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 185-185: internal/parser/antlr/numscript_base_listener.go#L185
Added line #L185 was not covered by tests
[warning] 188-188: internal/parser/antlr/numscript_base_listener.go#L188
Added line #L188 was not covered by testsinternal/parser/antlr/numscript_lexer.go (5)
46-49: LGTM! Token literal definitions are correct.The addition of the 'oneof' token and the subsequent reindexing of token literals are consistent with the PR objective.
52-53: LGTM! Token symbolic names are properly aligned.The symbolic names array is correctly updated to maintain consistency with the literal names and token constants.
60-65: LGTM! Token rule names are properly defined.The rule names array is correctly updated to include the new 'oneof' token while maintaining proper indexing.
Line range hint
19-19: Address the TODO comment about EOF string.The TODO comment suggests that the EOF string definition is missing. Consider implementing this to ensure complete lexer functionality.
Would you like me to help implement the EOF string definition?
267-303: LGTM! Token constants are correctly updated.The token constants are properly incremented to accommodate the new 'oneof' token while maintaining consistency with the literal and symbolic names.
internal/parser/antlr/NumscriptLexer.tokens (2)
1-38: LGTM! Token ID assignments are properly sequenced.The token renumbering maintains consistent sequential ordering while accommodating the new 'oneof' token.
40-64: Verify token ID consistency across parser components.The literal mappings correctly reflect the updated token IDs. However, we should verify that these changes are consistently applied across related parser files.
✅ Verification successful
Token IDs are consistently defined across parser components
The token mappings, including 'oneof' (T__1=2), are consistently defined across all parser files. No mismatches or outdated token IDs were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify token ID consistency across parser files # Check for any mismatches in token IDs echo "Checking token ID consistency..." for file in $(fd -e interp -e java -e go -e tokens . internal/parser/antlr); do echo "=== $file ===" # Look for 'oneof' token definitions and their IDs rg -A 1 -B 1 "'oneof'|T__1" "$file" done # Verify no hardcoded old token IDs remain echo "Checking for old token IDs..." rg "T__0.*=.*2|WS.*=.*2|NEWLINE.*=.*3" internal/parser/antlr/Length of output: 3448
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/interpreter/interpreter_test.go (1)
3546-3668: Consider refactoringoneoftests into a table-driven format to reduce duplication.Refactoring these similar test cases into a table-driven test can enhance maintainability and make it easier to add new scenarios in the future.
numscript_test.go (1)
100-135: LGTM! Consider adding more test cases.The test implementation correctly verifies the basic functionality of the
oneofconstruct. However, consider adding test cases for:
- Error handling when the feature flag is not enabled
- Empty
oneofblock- Nested
oneofblocks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Numscript.g4(1 hunks)internal/interpreter/batch_balances_query.go(1 hunks)internal/interpreter/interpreter.go(7 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/numscript_parser.go(23 hunks)numscript_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/parser/antlr/Numscript.interp
- Numscript.g4
🔇 Additional comments (10)
internal/parser/antlr/numscript_parser.go (1)
2772-2850: Code changes correctly integrateSrcOneofContextfor theoneofconstruct.The implementation of
SrcOneofContextappropriately handles the newoneofgrammar rule, ensuring that the parser can correctly parse and interpret scripts using theoneofconstruct.internal/interpreter/batch_balances_query.go (1)
131-139: Correctly handleSourceOneofby recursively invokingfindBalancesQueries.The addition of the
parser.SourceOneofcase effectively integrates the newoneofconstruct into the balance query process, ensuring that balances are correctly calculated for all sub-sources within theoneof.internal/interpreter/interpreter_test.go (5)
3546-3570: TestTestOneofInSourceSendFirstBranchvalidatesoneofwhen the first branch succeeds.The test correctly verifies that the
oneofconstruct selects the first successful source (@a) when it allows unbounded overdraft.
3572-3595: TestTestOneofInSourcehandles first branch failure and second branch success inoneof.This test ensures that when the first source fails due to insufficient overdraft allowance, the
oneofconstruct correctly falls back to the second source (@world).
3597-3618: TestTestOneofAllFailingverifies error handling when alloneofbranches fail.The test appropriately checks that a
MissingFundsErris returned when none of the sources inoneofcan satisfy the transfer amount.
3620-3645: TestTestOneofInSendAllvalidatesoneofbehavior in asend allcontext.The test confirms that only the first successful source (
@s1) is used in thesend alloperation, as expected with theoneofconstruct.
3647-3668: TestTestOneofSingletonchecksoneofwith a single source for correct behavior.This test ensures that
oneofoperates correctly even when only one source is provided, verifying successful execution when the source has sufficient balance.internal/interpreter/interpreter.go (3)
182-185: LGTM! Clean feature flag declaration.The feature flags are well-organized and follow a consistent naming convention.
580-609: LGTM! Well-implemented backtracking logic.The implementation correctly:
- Tries each source in order
- Backtracks if a source doesn't provide the full amount
- Falls back to the last source if no previous source succeeds
1027-1034: LGTM! Clean feature flag check implementation.The helper method is well-implemented with:
- Clear error handling
- Consistent return type
- Reusable design
| case *parser.SourceOneof: | ||
| err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // we can safely access the first one because empty oneof is parsing err | ||
| first := source.Sources[0] | ||
| return s.sendAll(first) | ||
|
|
There was a problem hiding this comment.
Inconsistent behavior in sendAll method.
The oneof implementation in sendAll only uses the first source without trying alternatives. This differs from the behavior in trySendingUpTo where it tries each source until one succeeds. Consider making the behavior consistent by:
- Either documenting that
sendAllalways uses the first source - Or implementing the same backtracking logic as in
trySendingUpTo
There was a problem hiding this comment.
that's not inconsistent: this is indeed a backtracking operator, but when calling this method none of the branch can fail, hence we just forward to the first source
There was a problem hiding this comment.
@ascandone Thank you for the clarification. That makes perfect sense - since sources can't fail in "send all" context, using the first source is sufficient.
✏️ Learnings added
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.647Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cmd/run.go (1)
208-208: Enhance CLI flag documentation and test coverage.While the flag registration follows the established pattern, consider these improvements:
- Add test coverage for CLI flag parsing
- Enhance the help text to better explain what the "oneof combinator" does and its use cases
Example of more detailed help text:
-cmd.Flags().BoolVar(&oneOfFeatureFlag, interpreter.ExperimentalOneofFeatureFlag, false, "feature flag to enable the oneof combinator") +cmd.Flags().BoolVar(&oneOfFeatureFlag, interpreter.ExperimentalOneofFeatureFlag, false, "feature flag to enable the oneof combinator, which allows grouping multiple sources under a single source definition")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 208-208: internal/cmd/run.go#L208
Added line #L208 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/analysis/check.go(1 hunks)internal/cmd/run.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/cmd/run.go
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by tests
[warning] 208-208: internal/cmd/run.go#L208
Added line #L208 was not covered by tests
internal/analysis/check.go
[warning] 465-468: internal/analysis/check.go#L465-L468
Added lines #L465 - L468 were not covered by tests
🔇 Additional comments (2)
internal/analysis/check.go (1)
465-469: Implementation looks good but needs test coverage.The implementation correctly handles the new
SourceOneoftype by recursively checking each source, following the same pattern asSourceInorder. However, the code coverage report indicates that these lines are not covered by tests.Let's verify the test coverage and existing test patterns:
Would you like me to help generate test cases for the
SourceOneofimplementation?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 465-468: internal/analysis/check.go#L465-L468
Added lines #L465 - L468 were not covered by testsinternal/cmd/run.go (1)
30-30: LGTM!The feature flag declaration follows the established pattern and is appropriately placed with other flag declarations.
| if oneOfFeatureFlag { | ||
| featureFlags[interpreter.ExperimentalOneofFeatureFlag] = struct{}{} | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for the oneof feature flag in the cmd package
The feature flag is well integrated with the interpreter package and has thorough test coverage there. However, the cmd package lacks test coverage for:
- Flag parsing and validation
- Feature flag propagation to the interpreter
- Integration tests with the flag enabled/disabled
🔗 Analysis chain
Add test coverage for the oneof feature flag.
The feature flag implementation looks correct but lacks test coverage. Please add test cases to verify:
- Program behavior when the flag is enabled
- Program behavior when the flag is disabled
- Integration with the interpreter package's feature flag handling
Let's verify the feature flag integration with the interpreter package:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the interpreter package properly handles the oneof feature flag
# Check if the interpreter package defines the feature flag constant
rg -l "ExperimentalOneofFeatureFlag.*=.*\"" internal/interpreter/
# Check if there are corresponding feature flag checks in the interpreter
rg "ExperimentalOneofFeatureFlag" internal/interpreter/ -A 3 -B 3
# Look for test files that might need additional coverage
fd ".*test\.go$" internal/cmd/
Length of output: 3485
Script:
#!/bin/bash
# Check for feature flag testing in cmd package
rg "oneOfFeatureFlag" internal/cmd/ -A 3 -B 3
# Look for any test files that might contain feature flag tests
rg "TestRun.*Flag" internal/cmd/ -A 3 -B 3
# Check for any feature flag related test helpers
rg "testRun.*Flag" internal/cmd/ -A 3 -B 3
Length of output: 1402
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by tests
ascandone
force-pushed
the
experimental/oneof-combinator
branch
2 times, most recently
from
b9eba74 to
9f81e2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/analysis/check_test.go (1)
200-223: LGTM! Consider adding edge cases.The test effectively validates the basic unbound variable detection in oneof blocks.
Consider adding test cases for:
- Multiple unbound variables in the same oneof block
- Nested oneof blocks with unbound variables
- Mixed bound and unbound variables in the same oneof block
internal/parser/ast.go (1)
163-165: Consider adding validation methods for destination types.Both DestinationInorder and DestinationOneof share similar fields. Consider adding validation methods to ensure the Remaining field is properly set.
+func (d *DestinationInorder) Validate() error { + if d.Remaining == nil { + return errors.New("remaining clause is required") + } + return nil +} + +func (d *DestinationOneof) Validate() error { + if d.Remaining == nil { + return errors.New("remaining clause is required") + } + return nil +}Also applies to: 167-171
📜 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 (11)
Numscript.g4(2 hunks)internal/analysis/check.go(1 hunks)internal/analysis/check_test.go(1 hunks)internal/cmd/run.go(3 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/numscript_base_listener.go(2 hunks)internal/parser/antlr/numscript_listener.go(4 hunks)internal/parser/antlr/numscript_parser.go(24 hunks)internal/parser/ast.go(4 hunks)internal/parser/parser.go(4 hunks)internal/parser/parser_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cmd/run.go
- internal/analysis/check.go
- internal/parser/antlr/numscript_listener.go
- internal/parser/antlr/numscript_base_listener.go
🔇 Additional comments (8)
Numscript.g4 (2)
87-87: LGTM! Source oneof grammar is well-defined.The grammar for source oneof is clear and consistent with existing patterns.
97-100: Verify the requirement for REMAINING in destination oneof.The grammar enforces that destination oneof must have a REMAINING clause. This seems to differ from the regular destination rule which makes it optional.
internal/parser/ast.go (1)
106-109: LGTM! SourceOneof type is well-structured.The type definition follows the established pattern and correctly implements the Source interface.
internal/parser/antlr/Numscript.interp (1)
4-4: LGTM! Parser configuration updated correctly for the newoneofconstruct.The changes properly integrate the new
oneoftoken into the parser's configuration, with appropriate updates to token indices and ATN entries.Also applies to: 106-106
internal/parser/parser.go (3)
173-181: LGTM! Source parsing correctly handles the newoneofconstruct.The implementation follows the same pattern as other source types, properly collecting all sources from the context.
Line range hint
401-422: LGTM! Destination parsing updated to supportoneofconstruct.The changes correctly handle the new destination type while maintaining consistency with existing patterns.
448-449: LGTM! Return type updated for destination clause parsing.The change from
DestinationInorderClausetoCappedKeptOrDestinationaligns with the new grammar structure.internal/parser/antlr/numscript_parser.go (1)
2778-2856: LGTM! Auto-generated parser code.This is auto-generated code from ANTLR and correctly implements the parser rules for the new
oneofconstruct.Also applies to: 4103-4200
| func TestOneofSource(t *testing.T) { | ||
| p := parser.Parse(`send $amt ( | ||
| source = oneof { @s1 @s2 } | ||
| destination = @d | ||
| )`) | ||
| require.Empty(t, p.Errors) | ||
| snaps.MatchSnapshot(t, p.Value) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error cases for oneof source parsing.
The test covers the happy path but lacks error cases.
Consider adding tests for:
- Empty oneof block
- Invalid syntax within oneof
- Nested oneof blocks
| func TestOneofDestination(t *testing.T) { | ||
| p := parser.Parse(`send $amt ( | ||
| source = @s | ||
| destination = oneof { | ||
| max $m1 to @d1 | ||
| max $m2 kept | ||
| remaining to @d3 | ||
| } | ||
| )`) | ||
| require.Empty(t, p.Errors) | ||
| snaps.MatchSnapshot(t, p.Value) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add edge cases for oneof destination parsing.
While the basic functionality is tested, edge cases should be covered.
Consider adding tests for:
- Missing REMAINING clause
- Multiple REMAINING clauses
- Invalid max expressions
ascandone
force-pushed
the
experimental/oneof-combinator
branch
from
7e6572b to
b3352f3
Compare
No description provided.