-
Notifications
You must be signed in to change notification settings - Fork 5
docs: added examples #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughSeveral new NumScript test data files were added under Changes
Sequence Diagram(s)sequenceDiagram
participant Script
participant Source
participant FeeAccount
participant DestAccount
Script->>Source: Prepare $amt USD
Script->>FeeAccount: Send min($amt * $fee, $cap) USD
Script->>DestAccount: Send remaining USD
sequenceDiagram
participant Script
participant Src1
participant Src2
participant World
participant Dest1
participant Dest2
Script->>Src1: Send 10% of 77 USD
Script->>Src2: Send 20% of 77 USD
Script->>World: Send remaining 70% of 77 USD
Script->>Dest1: Send 50% of total
Script->>Dest2: Send 50% of total
sequenceDiagram
participant Script
participant Alice
participant Jon
Script->>Alice: Calculate max($limit - $jon_balance)
Script->>Jon: Send min($amt, max) EUR
sequenceDiagram
participant Script
participant World
participant Alice
Script->>World: Prepare $alice_overdraft EUR
Script->>Alice: Send $alice_overdraft EUR
sequenceDiagram
participant Script
participant World
participant Alice
participant Bob
Script->>World: Prepare $amt EUR
alt Alice overdraft not full
Script->>Alice: Top up overdraft
else Bob overdraft not full
Script->>Bob: Top up overdraft
else
Script->>World: Keep remaining
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (3)
✨ Finishing Touches🧪 Generate unit tests
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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
==========================================
+ Coverage 70.62% 71.32% +0.70%
==========================================
Files 41 41
Lines 4677 4631 -46
==========================================
Hits 3303 3303
+ Misses 1222 1176 -46
Partials 152 152 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/interpreter/testdata/numscript-cookbook/top-up.num.specs.json
Outdated
Show resolved
Hide resolved
| number $x = 10 + 1 | ||
| } | ||
|
|
||
| set_tx_meta("key", $x/2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really aware of how this is used, but it seems weird to me to not have any postings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only to show that values can have arbitrary math expressions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/interpreter/testdata/numscript-cookbook/mixed-source-prefer-single-source.num (1)
6-13: Clarifyoneofpreference or add an inline commentThe third option in the
oneofblock is itself a group containing both@s1and@s2. Depending on the interpreter’s resolution rules, the meaning of “prefer single source” versus “fallback to both” might not be obvious to future readers (or even to the test maintainer six months from now). A short inline comment explaining the expected selection order would eliminate any ambiguity.internal/interpreter/testdata/numscript-cookbook/mixed-source-dest-allotment.num (1)
3-5: Double-check mixed units in allotment blockPercentage clauses (
10%,20%) are followed by aremainingcatch-all, while the destination block below switches to fractional notation (1/2). Although the DSL allows this mix, consider using the same notation in both blocks for consistency (e.g.,50%instead of1/2) unless the divergence is intentional for coverage.internal/interpreter/testdata/numscript-cookbook/top-up-max.num (1)
12-13: Minor style polish & uninitialised variable reminder
- There is a trailing blank after
@jonthat you might want to trim to keep the examples tidy.$amtis declared but never initialised inside this snippet; ensure the harness injects it, otherwise the example could be misleading.- destination = @jon + destination = @joninternal/interpreter/testdata/numscript-cookbook/max-fee.num (1)
7-15: Clarify dual “remaining to @DesT” to avoid reader confusionThe script is correct—the inner
remaininghandles the leftover of the fee portion while the outerremainingroutes the leftover of the overall amount—but the identical wording can be mis-read as duplicating the transfer.
A short inline comment or slightly different wording (e.g.,leftovervsremaining) would improve readability for cookbook consumers.internal/interpreter/testdata/numscript-cookbook/top-up.num (1)
5-8: Consider explicit currency for consistencyElsewhere in the cookbook you use the
[CUR xxx]syntax. Writingsend [EUR $alice_overdraft] (would keep the examples stylistically uniform and remove any doubt about the variable’s currency.
internal/interpreter/testdata/numscript-cookbook/top-up-many.num (1)
8-12: Tiny style nit – align indentation insideoneofFor visual parity with other cookbook files, indent the three options by two spaces:
- destination = oneof { - max overdraft(@alice, EUR) to @alice - max overdraft(@bob, EUR) to @bob - remaining kept - } + destination = oneof { + max overdraft(@alice, EUR) to @alice + max overdraft(@bob, EUR) to @bob + remaining kept + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
internal/interpreter/testdata/numscript-cookbook/max-fee.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/meta-calc.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/mixed-source-dest-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/mixed-source-prefer-single-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/send-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/top-up-many.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/top-up-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/top-up.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/transfer-example.num.specs.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
internal/interpreter/testdata/numscript-cookbook/max-fee.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/meta-calc.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/mixed-source-dest-allotment.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/mixed-source-prefer-single-source.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/send-max.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/top-up-many.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/top-up-max.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/top-up.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/transfer-example.num(1 hunks)
⏰ 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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (3)
internal/interpreter/testdata/numscript-cookbook/meta-calc.num (1)
5-5: Confirm meta value type is accepted
set_tx_meta("key", $x/2)passes a numeric expression. If downstream consumers expect a string, you may want to wrap it withstr()(or the DSL equivalent) to avoid implicit type assumptions.internal/interpreter/testdata/numscript-cookbook/send-max.num (1)
1-4: LGTM – concise coverage of themaxsource clauseThe example cleanly exercises the
maxconstraint with an unrestricted destination. No issues spotted.internal/interpreter/testdata/numscript-cookbook/transfer-example.num (1)
8-9: Verify that@walletcontains the overdraft instrument
max $bank_overdraft from @walletassumes@walletis the ledger account holding the bank-overdraft asset. Double-check the surrounding test setup to ensure the balance really sits in@wallet; otherwise the interpreter will raise an “insufficient funds” error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/interpreter/testdata/numscript-cookbook/experimental/top-up.num (1)
2-2: Consider edge case documentation.While the implementation is correct, it would be valuable to document the behavior when
overdraft(@alice, EUR)returns 0 (no overdraft). Does the script still execute a send of 0 amount, or should this be handled differently?internal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num (1)
2-6: Consider consistency with monetary variable approach.This script uses a
numbervariable with currency specified in the send operation[EUR $amt], while the relatedtop-up.numuses amonetaryvariable. Consider documenting why different approaches are used or standardizing the pattern for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
internal/interpreter/testdata/numscript-cookbook/experimental/meta-calc.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/mixed-source-prefer-single-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/transfer-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/max-fee.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/mixed-source-dest-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/send-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/top-up-max.num.specs.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
internal/interpreter/testdata/numscript-cookbook/experimental/meta-calc.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/experimental/mixed-source-prefer-single-source.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/experimental/top-up.num(1 hunks)internal/interpreter/testdata/numscript-cookbook/experimental/transfer-example.num(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- internal/interpreter/testdata/numscript-cookbook/experimental/meta-calc.num
- internal/interpreter/testdata/numscript-cookbook/experimental/mixed-source-prefer-single-source.num
- internal/interpreter/testdata/numscript-cookbook/experimental/transfer-example.num
⏰ 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). (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (2)
internal/interpreter/testdata/numscript-cookbook/experimental/top-up.num (1)
1-8: LGTM! Clear and concise overdraft top-up example.The script demonstrates a straightforward overdraft top-up scenario where Alice's overdraft amount is calculated and then transferred from the world entity. The syntax is clean and the logic is easy to follow.
internal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num (1)
6-13: Excellent demonstration of prioritized overdraft allocation.The
oneofconstruct effectively demonstrates how to handle multiple overdraft top-ups with priority ordering. The logic clearly shows Alice gets priority over Bob, and any remaining funds are kept rather than sent elsewhere.
| vars { | ||
| number $amt | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Uninitialized variable will cause runtime error.
The variable $amt is declared but never initialized. This will likely cause a runtime error when the script attempts to execute the send operation.
Add an initialization value to fix this issue:
vars {
- number $amt
+ number $amt = 100
}Or specify the amount directly in the send operation and remove the variable if it's meant to be a parameter.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vars { | |
| number $amt | |
| } | |
| vars { | |
| number $amt = 100 | |
| } |
🤖 Prompt for AI Agents
In internal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num
at lines 1 to 3, the variable $amt is declared but not initialized, which will
cause a runtime error. Fix this by either assigning an initial value to $amt in
the vars block or by removing the variable and specifying the amount directly in
the send operation if $amt is intended as a parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ascandone Got it! I understand that the uninitialized variable is intentional for this example.
🐰
╭─────╮
│ ◕ ◕ │ curious about NumScript behavior
╰─────╯
✏️ Learnings added
Learnt from: ascandone
PR: formancehq/numscript#88
File: internal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num:1-3
Timestamp: 2025-07-25T08:12:46.359Z
Learning: In NumScript test data files under internal/interpreter/testdata/numscript-cookbook/, variables can be declared without initialization as they may be intended as parameters or templates where values are supplied externally at runtime.
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.
No description provided.