Add TOML parsing, globals, and compliance checks#183
Merged
Conversation
- Add TOML built-ins, module imports, and globals injection - Add TOML test harness and toml-test CI job - Update docs and runtime wiring for TOML support
Contributor
📝 WalkthroughWalkthroughAdds end-to-end TOML support: parser unit and AST, TOML builtin and globals injection, .toml module imports, parser tests and harness, Jest module/builtin tests, docs updates, and CI toml-compliance workflow that builds and runs a GocciaTOMLCheck harness against toml-test v1.1.0. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as "GitHub Actions"
participant Build as "Build Job (fpc-cross)"
participant Artifact as "Artifact Storage"
participant TomlJob as "toml-compliance (matrix)"
participant Runner as "run_toml_test_suite.py"
participant Harness as "GocciaTOMLCheck"
CI->>Build: trigger cross-compile (include GocciaTOMLCheck per target)
Build->>Artifact: upload `gocciascript-${target}` and `GocciaTOMLCheck`
CI->>TomlJob: start per-target matrix job
TomlJob->>Artifact: download `gocciascript-${target}` and `GocciaTOMLCheck`
TomlJob->>Harness: select platform harness binary
TomlJob->>Runner: run `run_toml_test_suite.py --harness=./build/GocciaTOMLCheck`
Runner->>Harness: invoke harness per-case, collect results
Runner->>TomlJob: write `toml-test-results-<target>.json`
TomlJob->>Artifact: upload per-target JSON artifact
CI->>CI: downstream jobs wait for toml-compliance success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 80 improved · 🔴 141 regressed · 53 unchanged · avg -1.5% arraybuffer.js — Interp: 🟢 2, 🔴 12 · avg -7.7% · Bytecode: 🟢 12, 2 unch. · avg +6.7%
arrays.js — Interp: 🟢 2, 🔴 16, 1 unch. · avg -3.7% · Bytecode: 🟢 15, 4 unch. · avg +7.5%
async-await.js — Interp: 🔴 6 · avg -9.1% · Bytecode: 🟢 6 · avg +2.5%
classes.js — Interp: 🔴 31 · avg -7.7% · Bytecode: 🟢 8, 🔴 4, 19 unch. · avg +0.3%
closures.js — Interp: 🟢 1, 🔴 6, 4 unch. · avg -1.2% · Bytecode: 🟢 3, 🔴 1, 7 unch. · avg +1.2%
collections.js — Interp: 🟢 5, 🔴 4, 3 unch. · avg +2.2% · Bytecode: 🟢 9, 🔴 1, 2 unch. · avg +6.2%
destructuring.js — Interp: 🟢 1, 🔴 19, 2 unch. · avg -2.8% · Bytecode: 🟢 6, 🔴 8, 8 unch. · avg -0.9%
fibonacci.js — Interp: 🔴 3, 5 unch. · avg -1.2% · Bytecode: 🟢 7, 1 unch. · avg +4.4%
for-of.js — Interp: 🟢 7 · avg +3.9% · Bytecode: 🟢 6, 1 unch. · avg +17.7%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 7, 🔴 1, 12 unch. · avg +0.9% · Bytecode: 🟢 13, 🔴 1, 6 unch. · avg +3.0%
json.js — Interp: 🔴 17, 3 unch. · avg -4.7% · Bytecode: 🟢 6, 🔴 2, 12 unch. · avg +0.7%
jsx.jsx — Interp: 🟢 4, 🔴 8, 9 unch. · avg -0.7% · Bytecode: 🟢 3, 🔴 4, 14 unch. · avg +0.2%
modules.js — Interp: 🟢 9 · avg +7.5% · Bytecode: 🟢 8, 1 unch. · avg +9.7%
numbers.js — Interp: 🟢 3, 🔴 7, 1 unch. · avg -0.8% · Bytecode: 🟢 6, 🔴 1, 4 unch. · avg +3.4%
objects.js — Interp: 🟢 3, 🔴 1, 3 unch. · avg +3.2% · Bytecode: 🟢 2, 🔴 2, 3 unch. · avg -0.4%
promises.js — Interp: 🟢 12 · avg +5.6% · Bytecode: 🟢 4, 🔴 3, 5 unch. · avg +0.6%
regexp.js — Interp: 🟢 5, 🔴 2, 4 unch. · avg +1.1% · Bytecode: 🟢 3, 🔴 2, 6 unch. · avg +0.3%
strings.js — Interp: 🟢 9, 🔴 1, 1 unch. · avg +3.4% · Bytecode: 🟢 1, 🔴 1, 9 unch. · avg +0.6%
typed-arrays.js — Interp: 🟢 10, 🔴 7, 5 unch. · avg -0.4% · Bytecode: 🟢 17, 🔴 2, 3 unch. · avg +5.6%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
units/Goccia.ScriptLoader.Globals.Test.pas (1)
30-37: Add one CLI-level TOML globals smoke test too.These tests stop at
TGocciaEngine/TGocciaBytecodeBackend, so they won't catch regressions in the actualScriptLoader --globals=...tomldispatch path. The new format is user-facing and should get the same command-level coverage the workflow already gives JSON/module globals.As per coding guidelines "When a feature is exposed through a CLI tool or other documented user-facing API, prefer adding or extending command-level/workflow-level coverage instead of asserting internal helper behavior directly in Pascal tests".
Also applies to: 53-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.ScriptLoader.Globals.Test.pas` around lines 30 - 37, Add a CLI-level smoke test to SetupTests by registering a new test (e.g., Test('CLI injects globals from TOML', TestCLIInjectGlobalsFromTOML)) alongside TestDetectsTOMLGlobalsFileByExtension, TestEngineInjectGlobalsFromTOML and TestBytecodeBackendInjectGlobalsFromTOML; implement TestCLIInjectGlobalsFromTOML to invoke the ScriptLoader CLI entrypoint with the --globals=<toml file> argument (using the same helper the JSON/module CLI tests use), assert the process returns success and that globals were injected into the running script, and clean up any temp files so the test exercises the full dispatch path from ScriptLoader to TGocciaEngine/TGocciaBytecodeBackend rather than only testing internals.units/Goccia.TOML.pas (2)
444-466: Ownership transfer pattern in ParseDocument is correct but subtle.The try-finally correctly handles both success and exception cases: on success,
FRoot := nilprevents the finally block from freeing the returned node; on exception,FRoot.Freecleans up the partially-constructed tree. Consider adding a brief comment explaining this ownership transfer pattern for future maintainers.📝 Optional clarifying comment
try SkipBlankLinesAndComments; while not IsAtEnd do begin if PeekChar = '[' then ParseTableHeader else ParseKeyValuePair; SkipBlankLinesAndComments; end; + // Transfer ownership to caller; nil prevents finally from freeing Result := FRoot; FRoot := nil; finally🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.TOML.pas` around lines 444 - 466, Add a short clarifying comment inside TGocciaTOMLParser.ParseDocument explaining the ownership-transfer pattern: note that on success the method assigns Result := FRoot and sets FRoot := nil to avoid freeing the returned node in the finally block, while on exception the finally block calls FRoot.Free to clean up the partially-built tree; reference the Reset, SkipBlankLinesAndComments, PeekChar, ParseTableHeader, ParseKeyValuePair and the finally block where FRoot is freed to show exactly where the transfer occurs.
229-244: String building uses repeated concatenation.
StripUnderscoresbuilds the result string viaResult := Result + Cin a loop. For typical TOML numeric literals (short strings), this is acceptable. If TOML parsing becomes a hot path in the future, consider usingTStringBufferfor O(n) assembly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.TOML.pas` around lines 229 - 244, The current StripUnderscores function should avoid repeated string concatenation; update it to build the output in linear time by using a preallocated buffer or a TStringBuilder/TStringBuffer equivalent instead of appending per-character. Modify the body of StripUnderscores to reserve the needed capacity and write characters by index (or use TStringBuilder.Append when C <> '_') so the function performs O(n) work for typical TOML parsing hotspots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 302-334: Replace the hardcoded python3 invocations in the TOML
compliance steps with a cross-platform fallback like the examples job uses:
define PYTHON_BIN="$(command -v python3 || command -v python)" near the top of
the step and use "$PYTHON_BIN" instead of python3 for both the
run_toml_test_suite.py call and the embedded here-doc execution; keep existing
HARNESS usage and ensure the here-doc still executes via "$PYTHON_BIN - <<'PY'".
This will match the fallback pattern used elsewhere and work on Windows bash
runners where only python may be present.
In `@AGENTS.md`:
- Line 452: DefaultGlobals example is missing ggYAML so it diverges from the
runtime default set; update the list shown (the array containing ggGlobalNumber,
ggPromise, ggJSON, ggTOML, ggSymbol, ggSet, ggMap, ggPerformance, ggTemporal,
ggJSX, ggArrayBuffer) to include ggYAML as well so it matches the default set
defined in units/Goccia.Engine.pas; ensure ggYAML is inserted in the same
comma-separated form as the other gg* entries.
In `@docs/built-ins.md`:
- Line 176: Update the sentence that documents the toml-test rerun command so it
matches the rest of the PR: include the --harness=./build/GocciaTOMLCheck
argument in the command (i.e., use python3 scripts/run_toml_test_suite.py
--harness=./build/GocciaTOMLCheck) or explicitly state that the harness argument
is optional; modify the text in the same paragraph that currently mentions
python3 scripts/run_toml_test_suite.py to reflect this change so
docs/build-system.md and docs/built-ins.md are consistent.
In `@docs/testing.md`:
- Around line 692-723: Update the two fenced diagram blocks in docs/testing.md
to include the language tag "text" so markdownlint stops flagging them: change
the first triple-backtick block containing the build → test → artifacts diagram
to start with ```text and likewise change the second triple-backtick block under
"PR Workflow" (the build → test / benchmark → PR comment / examples diagram) to
start with ```text; no other content changes are needed.
In `@README.md`:
- Around line 307-308: Remove the hard-coded date and exact counts from the
README sentence that currently includes "2026-04-04" and "680 of 680" (the line
beginning "The TOML surface is now also checked..."); replace it with an
evergreen statement such as "The TOML surface is checked against the official
toml-test corpus; see CI or the generated test report for current pass/fail
counts" and keep the local rerun hint (`python3
scripts/run_toml_test_suite.py`). Move the precise snapshot details (date and
numeric results) out of README into CI artifacts, release notes, or a generated
report so the README remains up-to-date without manual edits.
In `@scripts/GocciaTOMLCheck.dpr`:
- Around line 130-145: The harness currently uses TStringList.LoadFromFile and
Source.Text which normalizes line endings and encoding; instead read the file as
a raw byte stream and feed that exact data to the parser. Replace the
TStringList/Source path around Parser.ParseDocument so you use
TFile.ReadAllBytes(ParamStr(1)) (or equivalent raw byte reader), convert the
bytes to a RawByteString/AnsiString without any encoding/CRLF normalization, and
call Parser.ParseDocument (or add a ParseDocumentFromBytes wrapper) with that
raw data; keep the existing variables Parser, Root and error handling intact.
In `@scripts/run_toml_test_suite.py`:
- Around line 77-112: The code currently treats any mapping with keys
{"type","value"} as a tagged scalar; change the guard so it only takes the
tagged-scalar path when both want["type"] and want["value"] are plain strings
(and likewise have["type"] and have["value"] are strings), otherwise fall back
to normal recursive comparison; update the condition around the block that
checks set(want.keys()) == {"type","value"} (and references
want_type/want_value, have_type/have_value and normalize_datetime_text) to
include isinstance checks for both want and have fields before executing the
tagged-scalar logic.
- Around line 35-38: The git clone call currently clones the default branch;
update the argument list in the block that creates temp_dir/checkout_dir and
calls run([...]) so the clone is pinned to the declared SUITE_VERSION (add
"--branch", SUITE_VERSION to the run([...]) arguments, keeping "--depth", "1");
reference the variables temp_dir, checkout_dir and the run(...) invocation when
making the change.
- Around line 223-226: In evaluate_suite, json.loads on stdout or
expected_json_path can raise JSONDecodeError and abort the whole run; wrap the
two json.loads (actual_json = json.loads(stdout or "null") and expected_json =
json.loads(expected_json_path.read_text())) in try/except JSONDecodeError and on
error set ok = False and message to a clear per-case failure note (e.g., "JSON
decode error parsing actual stdout" or "JSON decode error reading expected
fixture: <filename>") so the case is recorded as failed instead of raising; keep
subsequent logic that calls compare_tagged_json unchanged when parsing succeeds
so the summary file is still produced.
In `@units/Goccia.Builtins.TOML.pas`:
- Around line 79-83: The code currently catches all exceptions around
FParser.Parse(...) and converts them to a SyntaxError; change the except block
to only catch the TOML parser's specific exception type (e.g.,
ETOMLParseException or the actual exception class your TOML library uses) and
call ThrowSyntaxError(E.Message) for that case, while re-raising any other
exceptions (use "else raise;" or a plain "raise" for non-parser exceptions);
update the except clause around the
FParser.Parse(AArgs.GetElement(0).ToStringLiteral.Value) call to use "except on
ETOMLParseException do ..." and preserve normal raising for everything else.
In `@units/Goccia.Engine.Backend.pas`:
- Around line 238-264: InjectGlobalsFromTOML can silently no-op because
RegisterGlobal is a no-op when FInterpreter is nil; add a fail-fast guard at the
start of TGocciaBytecodeBackend.InjectGlobalsFromTOML that checks if
FInterpreter is nil and raises a TGocciaRuntimeError (or calls RegisterBuiltIns
if that is the intended bootstrap path) with a clear message about calling
RegisterBuiltIns/bootstrapping first; reference FInterpreter,
InjectGlobalsFromTOML, RegisterGlobal and RegisterBuiltIns so it's easy to
locate and either raise the error or invoke the bootstrap before
parsing/injecting globals.
In `@units/Goccia.TOML.pas`:
- Around line 1541-1542: Replace the use of DefaultFormatSettings in the
TryStrToFloat call with a local TFormatSettings that forces a dot as the decimal
separator: declare a local variable (e.g. FS: TFormatSettings), assign FS :=
DefaultFormatSettings, set FS.DecimalSeparator := '.', and then call
TryStrToFloat(NumberText, AValue, FS) (use the same pattern as in
Goccia.YAML.pas and JSONParser.pas); update the line containing Result :=
TryStrToFloat(NumberText, AValue, DefaultFormatSettings) accordingly.
---
Nitpick comments:
In `@units/Goccia.ScriptLoader.Globals.Test.pas`:
- Around line 30-37: Add a CLI-level smoke test to SetupTests by registering a
new test (e.g., Test('CLI injects globals from TOML',
TestCLIInjectGlobalsFromTOML)) alongside TestDetectsTOMLGlobalsFileByExtension,
TestEngineInjectGlobalsFromTOML and TestBytecodeBackendInjectGlobalsFromTOML;
implement TestCLIInjectGlobalsFromTOML to invoke the ScriptLoader CLI entrypoint
with the --globals=<toml file> argument (using the same helper the JSON/module
CLI tests use), assert the process returns success and that globals were
injected into the running script, and clean up any temp files so the test
exercises the full dispatch path from ScriptLoader to
TGocciaEngine/TGocciaBytecodeBackend rather than only testing internals.
In `@units/Goccia.TOML.pas`:
- Around line 444-466: Add a short clarifying comment inside
TGocciaTOMLParser.ParseDocument explaining the ownership-transfer pattern: note
that on success the method assigns Result := FRoot and sets FRoot := nil to
avoid freeing the returned node in the finally block, while on exception the
finally block calls FRoot.Free to clean up the partially-built tree; reference
the Reset, SkipBlankLinesAndComments, PeekChar, ParseTableHeader,
ParseKeyValuePair and the finally block where FRoot is freed to show exactly
where the transfer occurs.
- Around line 229-244: The current StripUnderscores function should avoid
repeated string concatenation; update it to build the output in linear time by
using a preallocated buffer or a TStringBuilder/TStringBuffer equivalent instead
of appending per-character. Modify the body of StripUnderscores to reserve the
needed capacity and write characters by index (or use TStringBuilder.Append when
C <> '_') so the function performs O(n) work for typical TOML parsing hotspots.
🪄 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: 452c8213-c5c3-4c99-9d28-33d2e7afe8c8
📒 Files selected for processing (26)
.github/workflows/ci.yml.gitignoreAGENTS.mdREADME.mdScriptLoader.dprdocs/build-system.mddocs/built-ins.mddocs/design-decisions.mddocs/embedding.mddocs/language-restrictions.mddocs/testing.mdscripts/GocciaTOMLCheck.dprscripts/run_toml_test_suite.pytests/built-ins/TOML/parse.jstests/language/modules/helpers/config.tomltests/language/modules/toml-import.jsunits/Goccia.Builtins.TOML.pasunits/Goccia.Engine.Backend.pasunits/Goccia.Engine.pasunits/Goccia.FileExtensions.pasunits/Goccia.Modules.Loader.pasunits/Goccia.Runtime.Bootstrap.pasunits/Goccia.ScriptLoader.Globals.Test.pasunits/Goccia.ScriptLoader.Globals.pasunits/Goccia.TOML.Test.pasunits/Goccia.TOML.pas
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/testing.md`:
- Around line 703-704: Update the docs text for the toml-compliance job to match
the workflow's resolver by noting the Python fallback: change the hardcoded
invocation reference to indicate the job runs scripts/run_toml_test_suite.py
using "python3 (or python on Windows)" or similar wording; reference the
toml-compliance job and the command pattern "python3
scripts/run_toml_test_suite.py --harness=...
--output=toml-test-results-<target>.json" and ensure the doc mentions the
workflow's behavior described in .github/workflows/ci.yml (python3 || python) so
troubleshooting and CI docs stay in sync.
In `@scripts/run_toml_test_suite.py`:
- Around line 110-119: The float comparison branch (using want_type, want_value,
have_value) currently treats -0.0 and 0.0 as equal; after converting to floats
and handling NaNs, add a signed-zero check: import math and when
float(want_lower) == float(have_lower) but both values are zero (e.g.,
float(...) == 0.0), use math.copysign(1.0, float(want_lower)) vs
math.copysign(1.0, float(have_lower)) and if the signs differ return False with
the same error message format (path or '<root>': expected float X, got Y);
otherwise continue to accept the values. Ensure math is imported at the top of
the module.
- Around line 73-81: The normalize_datetime_text function currently returns
datetime/date/time objects which truncate sub-microsecond precision; change it
to return normalized ISO-8601 strings instead (e.g., replace spaces/t/z as done,
adjust trailing Z to +00:00) so callers get the full original precision; update
any consumer that compares fixture values (the test comparison that uses
normalize_datetime_text) to compare these returned strings rather than parsed
datetime/time objects to preserve nanosecond-level differences.
🪄 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: f3fdbd24-b8b4-40e3-af15-3909d3da16fe
📒 Files selected for processing (12)
.github/workflows/ci.ymlAGENTS.mdREADME.mddocs/built-ins.mddocs/design-decisions.mddocs/embedding.mddocs/testing.mdscripts/GocciaTOMLCheck.dprscripts/run_toml_test_suite.pyunits/Goccia.Builtins.TOML.pasunits/Goccia.Engine.Backend.pasunits/Goccia.TOML.pas
✅ Files skipped from review due to trivial changes (3)
- docs/embedding.md
- README.md
- units/Goccia.TOML.pas
🚧 Files skipped from review as they are similar to previous changes (5)
- AGENTS.md
- units/Goccia.Engine.Backend.pas
- docs/built-ins.md
- units/Goccia.Builtins.TOML.pas
- docs/design-decisions.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/run_toml_test_suite.py (1)
350-369: Ensure temporary checkout cleanup runs on exceptions too.
temp_checkout.cleanup()currently runs only on the happy path. Ifcompile_harness(...),evaluate_suite(...), or output writing raises, temp directories can be left behind.♻️ Proposed refactor
def main() -> int: args = parse_args() repo_root = Path(__file__).resolve().parent.parent suite_dir, temp_checkout = ensure_suite_checkout(args.suite_dir) - if args.harness is not None: - harness_path = args.harness.resolve() - if not harness_path.is_file(): - raise FileNotFoundError(f"TOML harness not found: {harness_path}") - else: - harness_path = compile_harness(repo_root, args.build_dir.resolve()) - report = evaluate_suite(harness_path, suite_dir.resolve(), args.timeout) - - if args.output is not None: - args.output.parent.mkdir(parents=True, exist_ok=True) - args.output.write_text(json.dumps(report, indent=2) + "\n", encoding="utf-8") - - print(json.dumps(report["summary"], indent=2)) - if args.output is not None: - print(args.output.resolve()) - - if temp_checkout is not None: - temp_checkout.cleanup() - - summary = report["summary"] - if summary["failed"] > 0 or summary["timeouts"] > 0: - return 1 - - return 0 + try: + if args.harness is not None: + harness_path = args.harness.resolve() + if not harness_path.is_file(): + raise FileNotFoundError(f"TOML harness not found: {harness_path}") + else: + harness_path = compile_harness(repo_root, args.build_dir.resolve()) + report = evaluate_suite(harness_path, suite_dir.resolve(), args.timeout) + + if args.output is not None: + args.output.parent.mkdir(parents=True, exist_ok=True) + args.output.write_text(json.dumps(report, indent=2) + "\n", encoding="utf-8") + + print(json.dumps(report["summary"], indent=2)) + if args.output is not None: + print(args.output.resolve()) + + summary = report["summary"] + if summary["failed"] > 0 or summary["timeouts"] > 0: + return 1 + return 0 + finally: + if temp_checkout is not None: + temp_checkout.cleanup()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run_toml_test_suite.py` around lines 350 - 369, The temp checkout created by ensure_suite_checkout may not be cleaned up if an exception occurs in compile_harness, evaluate_suite, or output writing; wrap the workflow that uses temp_checkout (the calls to compile_harness, evaluate_suite, writing args.output, and printing) in a try/finally so that temp_checkout.cleanup() is always called in the finally block (only if temp_checkout is not None), keeping the same logic for harness_path and report variables; reference ensure_suite_checkout, compile_harness, evaluate_suite, args.output, and temp_checkout.cleanup when locating where to add the try/finally.docs/testing.md (1)
245-252: Clarify whichtoml-testcheckout--suite-dirshould target.Small doc hardening: add one sentence that
--suite-dirshould point to atoml-testcheckout frommain(the tree containingtests/files-toml-1.1.0) to avoid accidental reduced coverage from older/tag-only checkouts.📝 Suggested wording
For TOML 1.1.0 checks against the official `toml-test` corpus, run: @@ python3 scripts/run_toml_test_suite.py --suite-dir=/path/to/toml-test python3 scripts/run_toml_test_suite.py --harness=./build/GocciaTOMLCheck --output=tmp/toml-suite-results.json+When using
--suite-dir, point it to atoml-testcheckout frommain(the one containingtests/files-toml-1.1.0) to run the full corpus.</details> Based on learnings: `scripts/run_toml_test_suite.py` intentionally uses `toml-test` `main`; the `v1.1.0` tag exposes fewer cases and omits `tests/files-toml-1.1.0`. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/testing.mdaround lines 245 - 252, Add a clarifying sentence to the
TOML test invocation docs so users know the --suite-dir should point to a
toml-test checkout from main (the branch that contains tests/files-toml-1.1.0)
rather than a v1.1.0 tag-only checkout; update the example block in
docs/testing.md alongside the python3 scripts/run_toml_test_suite.py usage to
include this sentence and use the suggested wording (or equivalent) referencing
the tests/files-toml-1.1.0 path, and ensure the guidance mentions that
scripts/run_toml_test_suite.py expects the full main corpus for complete
coverage.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@docs/testing.md:
- Around line 245-252: Add a clarifying sentence to the TOML test invocation
docs so users know the --suite-dir should point to a toml-test checkout from
main (the branch that contains tests/files-toml-1.1.0) rather than a v1.1.0
tag-only checkout; update the example block in docs/testing.md alongside the
python3 scripts/run_toml_test_suite.py usage to include this sentence and use
the suggested wording (or equivalent) referencing the tests/files-toml-1.1.0
path, and ensure the guidance mentions that scripts/run_toml_test_suite.py
expects the full main corpus for complete coverage.In
@scripts/run_toml_test_suite.py:
- Around line 350-369: The temp checkout created by ensure_suite_checkout may
not be cleaned up if an exception occurs in compile_harness, evaluate_suite, or
output writing; wrap the workflow that uses temp_checkout (the calls to
compile_harness, evaluate_suite, writing args.output, and printing) in a
try/finally so that temp_checkout.cleanup() is always called in the finally
block (only if temp_checkout is not None), keeping the same logic for
harness_path and report variables; reference ensure_suite_checkout,
compile_harness, evaluate_suite, args.output, and temp_checkout.cleanup when
locating where to add the try/finally.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `c27cdbb3-88f0-4dca-b33f-36b8352fcc0d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7c99662c4535faedf3c043211635348d40fcfa3e and 3f348031b72ad227233c54e98df1546f8e4f4824. </details> <details> <summary>📒 Files selected for processing (11)</summary> * `README.md` * `docs/built-ins.md` * `docs/design-decisions.md` * `docs/language-restrictions.md` * `docs/testing.md` * `scripts/run_toml_test_suite.py` * `units/Goccia.Engine.pas` * `units/Goccia.FileExtensions.pas` * `units/Goccia.Modules.Loader.pas` * `units/Goccia.Runtime.Bootstrap.pas` * `units/Goccia.ScriptLoader.Globals.pas` </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * README.md * docs/language-restrictions.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * units/Goccia.Runtime.Bootstrap.pas * docs/design-decisions.md * units/Goccia.FileExtensions.pas </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
This was referenced Apr 5, 2026
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TOML.parse(...)as a first-class built-in and wire TOML into module imports and CLI globals loading.GocciaTOMLCheckcompliance harness for the officialtoml-test1.1.0 corpus.Fixes #177
Testing
python3 scripts/run_toml_test_suite.py --harness=./build/GocciaTOMLCheckacross the platform matrix.failed,false_accepts,false_rejects,valid_mismatches, andtimeoutsin the generated JSON summary../build/TestRunner testsand native unit-test execution.Summary by CodeRabbit
New Features
Documentation
Tests
Chores