Support reading ScriptLoader source from stdin#138
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded stdin support across tools: new input unit and tests; ScriptLoader and BenchmarkRunner accept and process source from stdin (or Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant SL as ScriptLoader / BenchmarkRunner
participant Input as Input Unit\n(IsStdinPath / ReadSourceFromText)
participant Parser as Parser/Compiler
participant Engine as Engine\n(Interpreter / Bytecode backend)
participant Output as Output\n(stdout / file)
User->>SL: Invoke with no args or `-` (stdin)
SL->>Input: IsStdinPath(path)
Input-->>SL: true
SL->>Input: ReadSourceFromText(stdin)
Input-->>SL: TStringList (source)
SL->>Parser: ParseSource(ASource, AFileName)
Parser-->>SL: AST / parse result
alt mode = bytecode
SL->>Parser: CompileSource(ASource, AFileName)
Parser-->>Engine: Bytecode module
Engine-->>Output: Emit bytecode / Run module (per flags)
else mode = interpreted
SL->>Engine: RunInterpreted(ASource, AFileName)
Engine-->>Output: Execution result to stdout
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
units/Goccia.ScriptLoader.Input.Test.pas (1)
39-43: Add a fixture without a terminating newline.The new CLI examples use
printf "const x = 2 + 2; x;", which does not append a trailing newline. This setup writes the source withTStringList.SaveToFile, so it only exercises a newline-terminated last line and won't catch EOF-on-last-line regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.ScriptLoader.Input.Test.pas` around lines 39 - 43, The current test uses TStringList.Create and SaveToFile (with TempFileName) which always writes a trailing newline on the last line; replace that SaveToFile usage so the fixture is written without a terminating newline — e.g. produce the file by writing the two lines joined with sLineBreak but not ending with sLineBreak using a raw writer (TFile.WriteAllText or similar) to TempFileName instead of SaveToFile; update the block around TStringList.Create / Add / SaveToFile to write "const x = 2 + 2;" + sLineBreak + "x;" directly to TempFileName so EOF has no trailing newline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ScriptLoader.dpr`:
- Around line 409-416: When argument parsing leaves Paths.Count = 0 we must not
immediately fall back to RunScriptFromStdin; instead detect and reject
unknown/unsupported flags first. Update the argument-parsing flow so that after
parsing you track whether any unrecognized options were seen (or whether
exclusive flags like --help/--version were passed) and if so print a clear
error/help message and exit non‑zero; only call RunScriptFromStdin when no
unknown flags were present and no explicit paths were provided. Modify the
branch around Paths, RunScriptFromStdin and RunScripts to perform this
validation before defaulting to stdin.
---
Nitpick comments:
In `@units/Goccia.ScriptLoader.Input.Test.pas`:
- Around line 39-43: The current test uses TStringList.Create and SaveToFile
(with TempFileName) which always writes a trailing newline on the last line;
replace that SaveToFile usage so the fixture is written without a terminating
newline — e.g. produce the file by writing the two lines joined with sLineBreak
but not ending with sLineBreak using a raw writer (TFile.WriteAllText or
similar) to TempFileName instead of SaveToFile; update the block around
TStringList.Create / Add / SaveToFile to write "const x = 2 + 2;" + sLineBreak +
"x;" directly to TempFileName so EOF has no trailing newline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4be2166-45cd-4e85-84c0-c5dc8a8428a6
📒 Files selected for processing (6)
README.mdScriptLoader.dprdocs/build-system.mddocs/embedding.mdunits/Goccia.ScriptLoader.Input.Test.pasunits/Goccia.ScriptLoader.Input.pas
Benchmark Results263 benchmarks Interpreted: 🟢 47 improved · 216 unchanged · avg +4.5% arraybuffer.js — Interp: 14 unch. · avg +0.5% · Bytecode: 14 unch. · avg -0.9%
arrays.js — Interp: 🟢 1, 18 unch. · avg +2.1% · Bytecode: 🟢 3, 🔴 1, 15 unch. · avg +2.1%
async-await.js — Interp: 6 unch. · avg +4.6% · Bytecode: 6 unch. · avg -4.2%
classes.js — Interp: 🟢 1, 30 unch. · avg +4.0% · Bytecode: 🟢 1, 🔴 1, 29 unch. · avg -2.1%
closures.js — Interp: 🟢 1, 10 unch. · avg +4.4% · Bytecode: 🔴 1, 10 unch. · avg -0.9%
collections.js — Interp: 🟢 2, 10 unch. · avg +4.9% · Bytecode: 🟢 3, 9 unch. · avg +2.8%
destructuring.js — Interp: 🟢 5, 17 unch. · avg +5.2% · Bytecode: 🟢 3, 🔴 1, 18 unch. · avg -0.9%
fibonacci.js — Interp: 8 unch. · avg +2.9% · Bytecode: 🔴 1, 7 unch. · avg -2.0%
for-of.js — Interp: 🟢 7 · avg +8.7% · Bytecode: 7 unch. · avg -0.6%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 5, 15 unch. · avg +5.7% · Bytecode: 🔴 1, 19 unch. · avg -2.0%
json.js — Interp: 🟢 1, 19 unch. · avg +4.4% · Bytecode: 20 unch. · avg +0.4%
jsx.jsx — Interp: 🟢 2, 19 unch. · avg +5.1% · Bytecode: 🔴 4, 17 unch. · avg -5.3%
modules.js — Interp: 9 unch. · avg +0.7% · Bytecode: 9 unch. · avg +2.1%
numbers.js — Interp: 🟢 3, 8 unch. · avg +5.9% · Bytecode: 11 unch. · avg +0.5%
objects.js — Interp: 🟢 2, 5 unch. · avg +5.5% · Bytecode: 7 unch. · avg -0.9%
promises.js — Interp: 🟢 6, 6 unch. · avg +7.0% · Bytecode: 12 unch. · avg -2.8%
strings.js — Interp: 🟢 2, 9 unch. · avg +4.9% · Bytecode: 11 unch. · avg -1.3%
typed-arrays.js — Interp: 🟢 9, 13 unch. · avg +5.2% · Bytecode: 🟢 4, 18 unch. · avg +2.4%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Suite Timing
Measured on ubuntu-latest x64. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
332-346: Please cover the remaining stdin paths in CI.These smoke tests verify interpreted and bytecode execution, but the new
Goccia.ScriptLoader.Input.Testbinary is still only built—not run—and stdin--emit --output=...is the one new ScriptLoader flow still missing automated coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 332 - 346, CI lacks execution of the new Goccia.ScriptLoader.Input.Test binary and the ScriptLoader stdin flow that uses --emit --output; update the workflow to run the built Goccia.ScriptLoader.Input.Test binary (e.g., ./build/Goccia.ScriptLoader.Input.Test) to exercise its stdin path, and add a smoke step that pipes a small script into ./build/ScriptLoader using the --emit --output=... flag (pointing at a temp file), then assert the emitted output file exists and contains expected content (or that subsequent bytecode run produces "Result: 4"); reference Goccia.ScriptLoader.Input.Test and the ScriptLoader --emit --output stdin flow when adding these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ScriptLoader.dpr`:
- Around line 282-289: RunScriptFromFile is unconditionally loading every
AFileName as text which corrupts .gbc bytecode; before creating/LoadFromFile,
detect the file extension of AFileName (case-insensitive) and if it's ".gbc"
call RunBytecodeFromFile(AFileName) and return; only for non-.gbc paths continue
to create the TStringList, LoadFromFile and call RunSource(Source, AFileName).
Ensure you perform the extension check early in RunScriptFromFile so bytecode
files never go through TStringList.LoadFromFile.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 332-346: CI lacks execution of the new
Goccia.ScriptLoader.Input.Test binary and the ScriptLoader stdin flow that uses
--emit --output; update the workflow to run the built
Goccia.ScriptLoader.Input.Test binary (e.g.,
./build/Goccia.ScriptLoader.Input.Test) to exercise its stdin path, and add a
smoke step that pipes a small script into ./build/ScriptLoader using the --emit
--output=... flag (pointing at a temp file), then assert the emitted output file
exists and contains expected content (or that subsequent bytecode run produces
"Result: 4"); reference Goccia.ScriptLoader.Input.Test and the ScriptLoader
--emit --output stdin flow when adding these checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 842e4df1-2a90-4a20-9c24-af2a3d96fb3c
📒 Files selected for processing (10)
.github/workflows/ci.yml.github/workflows/pr.ymlAGENTS.mdBenchmarkRunner.dprREADME.mdScriptLoader.dprdocs/benchmarks.mddocs/build-system.mddocs/embedding.mdunits/Goccia.ScriptLoader.Input.Test.pas
✅ Files skipped from review due to trivial changes (4)
- docs/benchmarks.md
- README.md
- .github/workflows/pr.yml
- docs/build-system.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/embedding.md
Summary
Testing
Closes #129
Summary by CodeRabbit
New Features
Documentation
Tests / CI