feat: cli, change run args #84
Conversation
|
""" WalkthroughThe command-line interface for running a numscript file was refactored to consolidate all input sources into a single JSON inputs file, replacing multiple input flags and stdin handling. The LSP command handler was modified to retain the Changes
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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 (
|
ascandone
force-pushed
the
cli/change-run-args
branch
from
2b822c6 to
b6e1e2b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
==========================================
+ Coverage 71.06% 71.32% +0.25%
==========================================
Files 41 41
Lines 4676 4631 -45
==========================================
- Hits 3323 3303 -20
+ Misses 1212 1176 -36
- Partials 141 152 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
🧹 Nitpick comments (2)
internal/cmd/run.go (2)
39-43: Consider returning the actual parsing errors.The function prints parsing errors to stderr but returns a generic "parsing failed" error. Consider including more context in the returned error.
parseResult := parser.Parse(string(numscriptContent)) if len(parseResult.Errors) != 0 { fmt.Fprint(os.Stderr, parser.ParseErrorsToString(parseResult.Errors, string(numscriptContent))) - return fmt.Errorf("parsing failed") + return fmt.Errorf("parsing failed with %d errors", len(parseResult.Errors)) }
132-132: Fix grammatical error in command description.There's a grammatical error in the help text.
-You can use explicitly specify where the inputs file should be using the optional --inputs argument. +You can explicitly specify where the inputs file should be using the optional --inputs argument.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cmd/lsp.go(2 hunks)internal/cmd/run.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ascandone
PR: formancehq/numscript#70
File: internal/utils/pretty_csv.go:12-16
Timestamp: 2025-07-16T14:49:54.326Z
Learning: ascandone prefers documenting function preconditions in comments rather than adding defensive validation code when the precondition violation would cause a panic. This is a valid design choice for utility functions.
⏰ 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). (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (1)
internal/cmd/run.go (1)
21-31: Well-structured input definitions.The consolidation of inputs into a single
InputsFilestruct provides a clean and organized approach to handling all input types. The JSON tags are properly defined.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ascandone
force-pushed
the
cli/change-run-args
branch
from
1a04fe9 to
ab32999
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ascandone
force-pushed
the
cli/change-run-args
branch
2 times, most recently
from
f3f1c6f to
91a284f
Compare
ascandone
force-pushed
the
cli/change-run-args
branch
from
91a284f to
2cc29f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/cmd/run.go (1)
49-52: Add helpful error message when inputs file is not found.When the inputs file doesn't exist, the error message might not be clear to users. Consider wrapping the error with more context.
inputsContent, err := os.ReadFile(inputsPath) if err != nil { - return err + return fmt.Errorf("failed to read inputs file '%s': %w", inputsPath, err) }
🧹 Nitpick comments (1)
internal/cmd/run.go (1)
124-124: Fix typo in help text.There's a grammatical error in the help text.
-You can use explicitly specify where the inputs file should be using the optional --inputs argument. +You can explicitly specify where the inputs file should be using the optional --inputs argument.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
inputs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
internal/cmd/lsp.go(2 hunks)internal/cmd/run.go(2 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: ascandone
PR: formancehq/numscript#70
File: internal/utils/pretty_csv.go:12-16
Timestamp: 2025-07-16T14:49:54.326Z
Learning: ascandone prefers documenting function preconditions in comments rather than adding defensive validation code when the precondition violation would cause a panic. This is a valid design choice for utility functions.
internal/cmd/run.go (2)
Learnt from: ascandone
PR: #31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Learnt from: ascandone
PR: #27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the internal/interpreter package (e.g., reconciler_test.go and interpreter_test.go), it's acceptable to use hardcoded "<kept>" strings in test data and comments, and they do not need to be replaced with the KEPT_ADDR constant.
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cmd/lsp.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ascandone
PR: formancehq/numscript#70
File: internal/utils/pretty_csv.go:12-16
Timestamp: 2025-07-16T14:49:54.326Z
Learning: ascandone prefers documenting function preconditions in comments rather than adding defensive validation code when the precondition violation would cause a panic. This is a valid design choice for utility functions.
internal/cmd/run.go (2)
Learnt from: ascandone
PR: #31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Learnt from: ascandone
PR: #27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the internal/interpreter package (e.g., reconciler_test.go and interpreter_test.go), it's acceptable to use hardcoded "<kept>" strings in test data and comments, and they do not need to be replaced with the KEPT_ADDR constant.
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
internal/cmd/run.go (6)
20-25: Well-structured inputs consolidation.The
InputsFilestruct provides a clean way to consolidate all input sources into a single JSON structure. The field types and JSON tags are appropriate for the use case.
27-30: Good simplification of command arguments.The simplified
RunArgsstruct reduces complexity by consolidating multiple input flags into a single inputs file path, while maintaining the essential output format option.
32-42: Good error handling for parsing errors.The parsing error handling properly writes to stderr and includes source information, providing helpful feedback to users when numscript parsing fails.
65-78: Excellent execution error handling with source ranges.The execution error handling provides comprehensive feedback by showing both the error message and the source range where the error occurred, making debugging much easier for users.
118-126: Clear documentation of new inputs file convention.The updated help text clearly explains the new inputs file naming convention and usage, making it easy for users to understand how to structure their inputs.
138-139: Good flag simplification.The simplified flag structure reduces complexity while maintaining essential functionality. The inputs flag provides flexibility when the default naming convention doesn't fit.
There was a problem hiding this comment.
Bug: Nil Maps in Struct Fields Cause Runtime Issues
The InputsFile struct's Variables, Meta, and Balances map fields are now nil if not present in the JSON input, instead of being initialized as empty maps. This can cause runtime panics or unexpected behavior when passed to interpreter.RunProgram, which expects non-nil maps.
internal/cmd/run.go#L53-L68
Lines 53 to 68 in 8d5fc12
Bug: Error Messages Concatenate with Shell Prompt
Error messages printed to os.Stderr in run.go lack a newline character, as fmt.Fprint is used instead of fmt.Fprintln. This causes messages to concatenate with the shell prompt, hindering readability and creating an inconsistency with lsp.go.
internal/cmd/run.go#L131-L132
Lines 131 to 132 in 8d5fc12
Was this report helpful? Give feedback by reacting with 👍 or 👎
ascandone
force-pushed
the
cli/change-run-args
branch
from
2e6b73a to
32d8914
Compare
| Meta: opt.Meta, | ||
| result, iErr := interpreter.RunProgram(context.Background(), parseResult.Value, inputs.Variables, interpreter.StaticStore{ | ||
| Balances: inputs.Balances, | ||
| Meta: inputs.Meta, |
There was a problem hiding this comment.
Bug: JSON Parsing Initializes Maps Incorrectly
The InputsFile struct's Variables, Meta, and Balances map fields are no longer explicitly initialized. Unlike the previous implementation, these fields are now nil if missing from the JSON input file. This can lead to nil pointer dereferences or unexpected behavior when passed to interpreter.RunProgram(), which likely expects non-nil maps.
Locations (1)
| return lsp.RunServer() | ||
| err := lsp.RunServer() | ||
| if err != nil { | ||
| cmd.SilenceErrors = true |
There was a problem hiding this comment.
Discussion on the merit of Run vs RunE and Cobra error handling: https://numary.slack.com/archives/C096FH98AKC/p1753358540338769
* feat: change cli command args * feat: better docs and improve err handling * fix: fix error handling * feat: added schema for inputs file * feat: improve error output * feat: more reliable exit from cli (no explicit exit() call)
This PR updates and simplify the CLI args, so that the usage is simplified and more consistent with the playground.
Docs preview: https://formance-d105fefb-add-cli-docs.mintlify.app/modules/numscript/cli