feat: overdraft function #14
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 63.22% 63.24% +0.01%
==========================================
Files 29 29
Lines 6390 6456 +66
==========================================
+ Hits 4040 4083 +43
- Misses 2171 2191 +20
- Partials 179 182 +3 ☔ View full report in Codecov by Sentry. |
ascandone
force-pushed
the
feat/experiment-overdraft-function
branch
from
c26bdb0 to
d619d6e
Compare
ascandone
force-pushed
the
feat/experiment-overdraft-function
branch
from
d619d6e to
c5dcc88
Compare
ascandone
force-pushed
the
feat/experiment-overdraft-function
branch
from
8f1fd3a to
be5b6d9
Compare
ascandone
force-pushed
the
feat/experiment-overdraft-function
branch
from
8f26655 to
6d64890
Compare
WalkthroughThe changes in this pull request primarily enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Interpreter
participant OverdraftFeature
participant Parser
User->>CLI: Invoke command with overdraft flag
CLI->>Interpreter: Run program with feature flags
Interpreter->>OverdraftFeature: Check if overdraft feature is enabled
OverdraftFeature->>Parser: Parse function call with overdraft
Parser-->>Interpreter: Return parsed function call
Interpreter-->>CLI: Return results
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
internal/interpreter/interpreter.go (1)
180-183: Consider defining 'FeatureFlag' as a distinct typeCurrently,
FeatureFlagis defined as a type alias forstringusingtype FeatureFlag = string. If you intend to create a new distinct type, consider defining it astype FeatureFlag stringwithout the=. This can provide stronger type safety and prevent inadvertent assignments of plain strings.internal/interpreter/interpreter_test.go (1)
99-112: Consider passing an empty map instead ofnilforfeatureFlagsIn the
RunProgramcall (lines 103-112), passingnilforfeatureFlagscould risk anilmap dereference if future code changes access the map without checking fornil. Consider initializingfeatureFlagsas an empty map to enhance robustness.internal/analysis/check.go (1)
79-83: LGTM with minor documentation enhancementThe function signature and implementation look good. Consider enhancing the documentation to explicitly mention that this is an experimental feature.
- Docs: "get absolute amount of the overdraft of an account. Returns zero if balance is not negative", + Docs: "[experimental] get absolute amount of the overdraft of an account. Returns zero if balance is not negative",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Numscript.g4(1 hunks)internal/analysis/check.go(2 hunks)internal/cmd/run.go(3 hunks)internal/interpreter/interpreter.go(7 hunks)internal/interpreter/interpreter_error.go(1 hunks)internal/interpreter/interpreter_test.go(2 hunks)internal/parser/antlr/Numscript.interp(1 hunks)internal/parser/antlr/numscript_parser.go(8 hunks)internal/parser/parser.go(2 hunks)
🔇 Additional comments (16)
internal/parser/antlr/Numscript.interp (1)
104-104: No issues found with the token and ATN modifications
The changes to the Numscript.interp file involve updates to the tokens and ATN structure, which are consistent with the grammar enhancements. No issues detected.
internal/interpreter/interpreter.go (6)
143-149: Handling 'overdraft' origin is properly implemented
The code correctly adds a case for FnVarOriginOverdraft in the handleOrigin method, calling the overdraft function and returning the result appropriately.
203-206: Feature flag for overdraft function is correctly implemented
The code checks for the ExperimentalOverdraftFunctionFeatureFlag in featureFlags and sets the OverdraftFunctionFeatureFlag field appropriately in the programState.
264-264: Added 'OverdraftFunctionFeatureFlag' field to 'programState'
The boolean field OverdraftFunctionFeatureFlag is added to track the enabled state of the overdraft function feature flag within the interpreter state.
798-813: Added 'getBalance' utility function for code reuse
The getBalance function encapsulates the logic for fetching and caching account balances, promoting code reuse and improving maintainability.
829-832: Refactored 'balance' function to use 'getBalance' utility
The balance function now utilizes the getBalance utility function, reducing code duplication and enhancing readability.
851-887: Implemented 'overdraft' function with proper feature flag check
The overdraft function correctly checks the OverdraftFunctionFeatureFlag before executing. It parses the account and asset arguments, retrieves the balance using getBalance, and returns the absolute value of the negative balance as the overdraft amount. The logic appropriately handles positive, zero, and negative balances.
internal/interpreter/interpreter_test.go (2)
85-86: Refactored 'test' function to use 'testWithFeatureFlag' for consistency
The test function now calls testWithFeatureFlag with an empty feature flag, promoting code reuse and ensuring consistent test handling whether feature flags are used or not.
3303-3412: Added comprehensive tests for 'overdraft' function
The test cases TestOverdraftFunctionWhenNegative, TestOverdraftFunctionWhenZero, and TestOverdraftFunctionWhenPositive effectively verify the overdraft function's behavior under various balance conditions. The use of testWithFeatureFlag appropriately checks feature flag enforcement.
Numscript.g4 (1)
63-64: Updated 'functionCall' rule to include 'OVERDRAFT' as a valid function name
The grammar modification allows OVERDRAFT to be recognized as a function name, enabling the parser to correctly handle overdraft function calls.
internal/interpreter/interpreter_error.go (1)
187-195: LGTM: Well-structured error type for experimental features
The new ExperimentalFeature error type is well-implemented, following the established pattern of other error types in the file. The error message clearly communicates the need for a feature flag.
internal/cmd/run.go (3)
29-29: LGTM: Clear feature flag declaration
The feature flag variable follows the package's naming conventions and has clear intent.
120-124: LGTM: Proper feature flag handling
The feature flags map is correctly initialized and populated when the overdraft feature is enabled.
202-204: LGTM: Well-documented flag registration
The feature flag is properly registered with a clear description and safe default value (false).
internal/parser/parser.go (1)
Line range hint 511-524: LGTM: Robust function name parsing
The updated parseFnCall implementation correctly handles the new grammar structure, properly extracting function names from either OVERDRAFT tokens or identifiers while maintaining proper nil checks and error handling.
internal/analysis/check.go (1)
79-83: Add feature flag validation
Since this is an experimental feature (as mentioned in PR objectives), consider adding validation in the type checking phase to ensure the function is only used when the feature flag is enabled.
Let's verify if similar feature flag validations exist in the codebase:
Added an overdraft() function which returns the absolute value of the negative balance of the given account and currency; 0 if overdraft is non negative (see tests as reference)
This is a new numscript functionality which is not implemented in the current machine implementation; thus the functionality is under the
experimental-overdraft-functionfeature flag