Add TOML file parsing functionality #164
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds TOML configuration support by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client/CLI
participant UFS as ufs.File
participant Parser as parser
participant TOML as go-toml/v2
participant Replacer as IterateOverJson
CLI->>Parser: Parse(file, type=Toml)
Parser->>UFS: Open file
UFS-->>Parser: file handle
Parser->>TOML: Unmarshal -> map[string]interface{}
TOML-->>Parser: data map
Parser->>Parser: dyno.ConvertMapI2MapS / normalizeTomlTypes
Parser->>Replacer: IterateOverJson(map)
Replacer-->>Parser: replaced map
Parser->>TOML: Marshal -> TOML bytes
TOML-->>Parser: TOML bytes
Parser->>UFS: Truncate/seek & write bytes
UFS-->>Parser: write result
Parser-->>CLI: return result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
parkervcp
left a comment
parkervcp
left a comment
There was a problem hiding this comment.
LGTM. I can appreciate some code reuse.
|
@vanlueckn If you want to make sure the build in the action for this works. I don't have anything that uses toml to est with. if @Kotori0629 could also validate that would be good. |
I will check the build and toml parsing in a few minutes. I have a rust based software running which uses toml config files. |
|
Does work for me |
|
This looks fine to me but just a question can you try it with the applied changes from #161 as json and yaml parsing can do strange things. If not that is also fine |
|
It's working normally! thx! |
|
@vanlueckn can you attempt to resolve conflicts with the json pr I just merged please. |
|
Done |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@parser/parser.go`:
- Around line 510-514: The variable named data is actually the []byte returned
by IterateOverJson and has no Data() method, causing a compile error; fix by
decoding that JSON bytes into an interface{} (like in parseYamlFile) and then
pass that decoded value to toml.Marshal instead of calling data.Data(); locate
the call site where IterateOverJson is used (variable "data") and replace the
toml.Marshal(data.Data()) step with unmarshalling the JSON bytes into var v
interface{} (json.Unmarshal) and then toml.Marshal(v).
🧹 Nitpick comments (1)
parser/parser.go (1)
495-503:dyno.ConvertMapI2MapSis a no-op for TOML — consider dropping it.
pelletier/go-toml/v2unmarshals intomap[string]interface{}natively (unlikegopkg.in/yaml.v3which can producemap[interface{}]interface{}), so thedyno.ConvertMapI2MapScall is unnecessary here. It's harmless, but removing it would avoid misleading future readers into thinking TOML needs the same fixup as YAML.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go.modparser/parser.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
parser/parser.go (1)
internal/ufs/file.go (1)
File(17-83)
🔇 Additional comments (2)
parser/parser.go (2)
17-17: LGTM — Import, constant, and switch case follow existing patterns.The new
tomlimport,Tomlconstant, andParseswitch case are consistent with how the other formats (YAML, JSON, INI, XML) are wired.Also applies to: 34-34, 233-234
489-528: Seek+Truncate should happen before the write, not after marshaling — order is correct, but verify partial-failure safety.The seek/truncate/write sequence (lines 517–526) mirrors the other parsers and is correctly placed after all processing succeeds. The error wrapping message on line 525 (
"parser: failed to write toml file to disk") is appropriately TOML-specific (unlike the YAML/text parsers that still say"properties file"— a pre-existing issue, not yours to fix).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@parser/parser.go`:
- Around line 487-533: parseTomlFile currently does a TOML → JSON →
JSON-unmarshal→TOML round-trip which causes numeric/datetime type fidelity loss
(integers become float64 and datetimes become strings); to fix, after
json.Unmarshal into jsonData in parseTomlFile (and before toml.Marshal) walk the
resulting structure returned by IterateOverJson and convert any float64 that
represent whole numbers back to an integer type (e.g., int64) and normalize
datetime strings if needed, or alternatively replace the json.Unmarshal step
with a json.Decoder using UseNumber() and ensure toml marshaling uses a mode
that preserves JSON numbers; update parseTomlFile (and related helpers used by
IterateOverJson if necessary) to perform this conversion so toml.Marshal emits
correct integer types.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
parser/parser.go
🧰 Additional context used
🧬 Code graph analysis (1)
parser/parser.go (1)
internal/ufs/file.go (1)
File(17-83)
🔇 Additional comments (3)
parser/parser.go (3)
17-17: LGTM — TOML library import is appropriate.The
pelletier/go-toml/v2import is well-established and properly placed alongside the other format-specific libraries.
34-34: LGTM — constant follows established naming pattern.
233-234: LGTM — switch case follows the existing dispatch pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@parser/parser.go`:
- Around line 465-470: The YAML parsing path currently calls normalizeTomlTypes
after decoding (see decoder := json.NewDecoder(...), decoder.UseNumber(), and
decoder.Decode(&jsonData)), which can convert RFC3339-like strings to time.Time;
remove that call from the generic YAML/JSON decode path and instead invoke
normalizeTomlTypes only from parseTomlFile so only TOML parsing gets the time
coercion while keeping UseNumber() for numeric fidelity in all parsers.
🧹 Nitpick comments (1)
parser/parser.go (1)
539-570: RenamenormalizeTomlTypesto reflect its broader usage, and note TOML local date/time gaps.
The function is called from
parseYamlFiletoo, so the namenormalizeTomlTypesis misleading. Something likenormalizeJsonNumberTypesornormalizeDecodedTypeswould be clearer.TOML supports local dates (
2024-01-15), local times (10:30:00), and local datetimes (2024-01-15T10:30:00without offset) — these won't matchRFC3339orRFC3339Nanoand will remain strings after round-tripping. This is a known limitation worth documenting if not addressed now.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
parser/parser.go
🔇 Additional comments (1)
parser/parser.go (1)
234-235: LGTM — Toml case wired correctly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@parser/parser.go`:
- Around line 559-566: The string branch in parser.go currently attempts to
parse any string as RFC3339/RFC3339Nano which wrongly converts plain strings
that happen to match the date format; update the case string block (the code
that calls time.Parse for RFC3339Nano/RFC3339) to first perform a cheap strict
check (e.g., require the string contains a 'T' and either 'Z' or a timezone
offset sign '+'/'-', or other narrow pattern) and only then call time.Parse;
otherwise return the original typed string. This keeps the time.Parse attempts
but prevents false positives for ordinary strings without an explicit datetime
pattern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
parser/parser.go
🔇 Additional comments (3)
parser/parser.go (3)
234-235: LGTM!Clean integration into the existing
Parseswitch statement, consistent with the other parser cases.
539-558:normalizeTomlTypesnumber and recursive handling looks correct.The
json.Number→int64→float64→stringfallback chain and the recursive map/slice traversal are well-structured.
488-537: Implementation is correct and addresses prior review feedback.The
UseNumber()decoder preserves integer fidelity through the JSON round-trip. Sincegoccy/go-json'sNumberis a type alias ofencoding/json.Number(and the project uses v0.10.5, which includes the v0.9.8+ fix for UseNumber stream support), thenormalizeTomlTypesfunction's type assertion againstjson.Numberworks correctly. The seek/truncate/write pattern is consistent with the other parsers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| case string: | ||
| if timeVal, err := time.Parse(time.RFC3339Nano, typed); err == nil { | ||
| return timeVal | ||
| } | ||
| if timeVal, err := time.Parse(time.RFC3339, typed); err == nil { | ||
| return timeVal | ||
| } | ||
| return typed |
There was a problem hiding this comment.
Time string coercion may produce false positives on plain string values.
After the JSON round-trip, both TOML native datetimes and plain string values that happen to match RFC 3339 format are indistinguishable — they're all strings. This means a TOML value like event_id = "2024-01-15T10:30:00Z" (intentionally a string) would be silently converted to time.Time, altering the output type.
This is an inherent limitation of the JSON round-trip approach and probably acceptable for the game-config use case. Just worth being aware of if TOML configs with date-like string values are encountered.
🤖 Prompt for AI Agents
In `@parser/parser.go` around lines 559 - 566, The string branch in parser.go
currently attempts to parse any string as RFC3339/RFC3339Nano which wrongly
converts plain strings that happen to match the date format; update the case
string block (the code that calls time.Parse for RFC3339Nano/RFC3339) to first
perform a cheap strict check (e.g., require the string contains a 'T' and either
'Z' or a timezone offset sign '+'/'-', or other narrow pattern) and only then
call time.Parse; otherwise return the original typed string. This keeps the
time.Parse attempts but prevents false positives for ordinary strings without an
explicit datetime pattern.
4 participants
This resolves issue #146
The toml parser uses the aleady existing indirect dependency and promotes it to a direct depencency.
The logic reuses the json parser logic.
I added it because i need toml parsing for some games.
Summary by CodeRabbit
New Features
Chores