Conversation
…ion handling for ConditionalNode, ListNode, and FunctionSpec
- Introduced `DensityFn.go` with various density functions and exporters. - Added `UsageStr` implementations for specs to improve usability. - Improved serialization logic by implementing `UnpackBuilders`. - Refined `SimpleSerializer` and surface rules for better output. - Updated grammar to support extended syntax.
…ization - Refactored `DensityFn.go` with better serialization logic using `unpack` package. - Enhanced completion handling for `EnumSpec`, `ConditionSpec`, and `ListSpec`. - Added new builders and argument unpackers in the `unpack` package. - Improved grammar to support docstrings. - Refactored surfaces and density-related specs for streamlined handling. - Added support for `SetLabel` in `EnumSpec` for improved labeling flexibility. - Optimized filter text and sorting logic in completion items.
…e grammar - Refactored `Enum.go` to streamline logic and improve error handling. - Added `Tag.go` to introduce support for tags with namespaces and references. - Enhanced grammar with support for resource tags and improved parsing for various specs. - Updated serialization logic across multiple specs to utilize `unpack.Args` consistently. - Fixed builder arguments handling and improved anchor parsing with tagged arguments. - Added diagnostic warnings for incomplete or invalid references. - Refined examples and updated grammar lexer for extended syntax.
|
Caution Review failedThe pull request is closed. WalkthroughMassive refactor: replaces the legacy traversal/construct/parsing stack with a spec-driven AST and a single redesigned grammar, adds a spec framework (functions, lists, enums, references, blocks, conditionals), worldgen serializers/exporters (Noise, DensityFn, SurfaceRules, Dimensions), Project/file-tree APIs, LSP/diagnostic/location types, CLI spec output, many new lang/* spec/unpack files, and removes many legacy traversal/construct implementations, grammars, and tests. Changes
Sequence Diagram(s)%%{init: {"theme":"base","themeVariables":{"primaryColor":"#E8F6F9","actorBorder":"#1B6E86","noteBg":"#F6FCFD"}}}%%
sequenceDiagram
autonumber
participant User
participant CLI as "mms CLI"
participant Blocks as "lang.Blocks"
participant SpecGen as "lang/spec.GenerateSpecString"
User->>CLI: run `mms spec`
CLI->>Blocks: read registered BlockSpecs
CLI->>SpecGen: GenerateSpecString(Blocks)
SpecGen-->>CLI: markdown spec string
CLI-->>User: print markdown
%%{init: {"theme":"base","themeVariables":{"primaryColor":"#FFF7E9","actorBorder":"#9A5A00","noteBg":"#FFFBF5"}}}%%
sequenceDiagram
autonumber
participant Project as "project.NewProject"
participant AddFile as "Project.AddFile"
participant Parser as "Grammar Parser"
participant Spec as "spec Matchers"
participant Node as "Spec Nodes"
participant Export as "ToFileTreeLike"
participant FS as "lib.FileTreeLike"
Project->>AddFile: AddFile(path, content)
AddFile->>Parser: parse content (new grammar)
Parser->>Spec: match Block/Value/Function contexts
Spec-->>Node: produce FunctionNode/ListNode/ConditionalNode
Node->>Export: ToFileTreeLike / ToSerializable
Export->>FS: merge/write worldgen JSON files
FS-->>Project: returned FileTreeLike
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
♻️ Duplicate comments (1)
lang/SurfaceRules.go (1)
139-141: Check slice length before indexingThe guard reads
n.Arguments[0]before confirming the slice has an element. ......
But we already used `` for other comment. Should we mark as critical? yes because panic. Good. Now Spline comment:Point serialization drops data
...Need to explain interplay. Third comment referencing same lines 235 etc:Prior comment retract
...Let's craft. Also we raised recommended for init? comment posted earlier `suggest_recommended_refactor` for Spline derived? The previous comment we posted referencing lines 514-538? We flagged recommended guard but now we also raising major. Should we maintain both? The recommended comment earlier asked to guard builder args? Actually comment we posted earlier referencing lines 514-538 suggests verifying `builder.Arguments`. With new major comment for Spline we may convert to something else? Evaluate: we already posted comment on same block with `suggest_recommended_refactor`. That comment said "Add nil check before using builder argument" but w ambiguous argu. With new major we may set unstoppable? Maybe unify? Eh. Maybe we should escalate to major by editing new comment or re-late? The earlier comment is more general. We can keep both? That would be redundant. Maybe we can adjust new comment to rely on flagged major difference but referencing same lines? Eh we can't remove previous comment. Already there. The earlier comment is minor (lack of guard). We'll now add major comment focusing on actual bug (lack of `mms_arg` tags). Should be glimpsed. Let's craft the new comments now. Need to ensure referencing line ranges. Also the correction comment. Let's produce. Need to ensure instructions: Each comment subject bold, short body. For duplicates, the comment should note previous issue invalid. Let's craft. Add new comment 139-145 for length. Add new comment 440-465. Add duplicate comment 235-236. Ok. Also remember to update scratchpad done. Let's produce comments. First new comment:
🧹 Nitpick comments (7)
examples/test_files/simple_noise.mms (1)
1-5: Consider adding explanatory comments.This test file demonstrates the basic Noise syntax but lacks documentation explaining what the example illustrates or what the parameter values represent. Adding brief comments would help users understand the intended usage.
For example:
+// Example: Basic Noise definition with amplitudes Namespace test_files { Noise { + // Create noise with seed -1 and specify amplitude values MyNoise = Noise(-1).Amplitudes(1,2,0.5,-1) } }examples/test_files/surface_rules.mms (1)
8-8: Improve fluent-call readability.Stacking three chained calls on one line hurts scanability; consider putting each
.PiglinSafe(),.Infiniburn(...),.Height(...)on its own line to match the surrounding style..helix/config.toml (1)
9-9: Consider a safer log-clearing command.The shell command uses a hardcoded path that may not exist on all systems. If
~/.cache/helix/doesn't exist, the command will fail silently.Consider using a command that creates the directory if needed:
-o = [ ":sh echo \"\" > ~/.cache/helix/helix.log", ":reload" ] +o = [ ":sh mkdir -p ~/.cache/helix && echo \"\" > ~/.cache/helix/helix.log", ":reload" ]Alternatively, use
:sh truncate -s 0 ~/.cache/helix/helix.log 2>/dev/null || trueto suppress errors if the file doesn't exist.lang/spec/Generation.go (1)
31-86: Consider improving code block indentation.Lines 78-80 manually indent each line with
" "+linefor code blocks. While this produces valid markdown, the approach has a couple of considerations:
- The indentation is hardcoded to 4 spaces, which may not align with markdown formatting tools or linters that expect fenced code blocks without manual indentation.
- If
linealready contains indentation, this adds another layer.Consider removing the manual indentation since you're already using fenced code blocks (```):
- for _, line := range strings.Split(usage, "\n") { - out = append(out, " "+line) - } + for _, line := range strings.Split(usage, "\n") { + out = append(out, line) + }Alternatively, if the indentation is intentional for nested markdown structure, add a comment explaining why:
// Indent each line for proper nesting within parent section for _, line := range strings.Split(usage, "\n") { out = append(out, " "+line) }lang/spec/Tag.go (1)
28-64: Remove or properly track commented-out completion logic.Lines 28-64 contain a large block of commented-out completion code with a TODO comment on line 28. This creates maintenance debt and makes the codebase harder to navigate.
Choose one of the following approaches:
Option 1 (recommended): If the completion feature is planned but not yet implemented, create a GitHub issue to track it and reference that issue in a concise comment:
-// TODO: We don't have any way to store tags right now -//func (r *TagSpec) Complete(fileSource string, position protocol.Position, _ *string, symbols map[string]*Namespace) []protocol.CompletionItem { -// // Extract any prefix the user has already typed -// prefix := ExtractPrefixAtPosition(fileSource, position) -// -// start := protocol.Position{ -// Line: position.Line, -// Character: protocol.UInteger(int(position.Character) - len(prefix)), -// } -// -// out := make([]protocol.CompletionItem, 0) -// for ns, nsSymbols := range symbols { -// for n, s := range nsSymbols.AllDecls() { -// if s.GetKind() == r.Kind { -// out = append(out, protocol.CompletionItem{ -// Label: fmt.Sprintf("[%s] %s:%s", s.GetKind(), ns, n), -// FilterText: &n, -// SortText: &n, -// TextEdit: protocol.TextEdit{ -// Range: protocol.Range{ -// Start: start, -// End: position, -// }, -// NewText: fmt.Sprintf("%s:%s", ns, n), -// }, -// Kind: &ReferenceKind, -// }) -// } -// } -// } -// -// // Filter by prefix if user has typed something -// return lo.Filter(out, func(item protocol.CompletionItem, index int) bool { -// parts := strings.Split(strings.ToLower(item.Label), ":") -// return strings.HasPrefix(parts[0], strings.ToLower(prefix)) || strings.HasPrefix(parts[1], strings.ToLower(prefix)) -// }) -//} +// TODO(#<issue-number>): Implement completion for tag references once tag storage is availableOption 2: If the code is truly obsolete, remove it entirely.
Would you like me to help create a GitHub issue to track the tag completion feature implementation?
lang/ast/Namespace.go (1)
55-59: Consider handling diagnostics in Merge.Line 57 calls
ns.Declare(name, symbol)but ignores the returned*Diagnostic. This silently drops duplicate identifier errors during namespace merging.Consider one of these approaches:
Option 1: Collect and return diagnostics if duplicate detection during merge is important:
-func (ns *Namespace) Merge(other *Namespace) { +func (ns *Namespace) Merge(other *Namespace) []Diagnostic { + var diagnostics []Diagnostic for name, symbol := range other.Declarations { - ns.Declare(name, symbol) + if diag := ns.Declare(name, symbol); diag != nil { + diagnostics = append(diagnostics, *diag) + } } + return diagnostics }Option 2: If duplicates during merge should be silently overwritten (last-wins), document this behavior and use direct map assignment:
func (ns *Namespace) Merge(other *Namespace) { + // Note: Duplicate declarations are overwritten (last-wins) for name, symbol := range other.Declarations { - ns.Declare(name, symbol) + ns.Declarations[name] = symbol } }lang/DensityFn.go (1)
514-538: Add nil check before using builder argumentIn
Splineinit the Point overload unconditionally indexesbuilder.Arguments. The tags expect[Location, Derivative, Value], but runtime shouldn't panic if user omits. Please guardlen(builder.Arguments)beforespec.NewNumberSpecmapping or when serializing points (lines 459-465 already, but assign expectsbuilder.Arguments). Verifyunpack.Args(point, builder.Arguments)gracefully handles short slices; otherwise add checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
go.sumis excluded by!**/*.sumlang/grammar/MinecraftMetascript.interpis excluded by!lang/grammar/*lang/grammar/MinecraftMetascript.tokensis excluded by!lang/grammar/*lang/grammar/MinecraftMetascriptLexer.interpis excluded by!lang/grammar/*lang/grammar/MinecraftMetascriptLexer.tokensis excluded by!lang/grammar/*lang/grammar/minecraftmetascript_base_listener.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_lexer.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_listener.gois excluded by!lang/grammar/*lang/grammar/minecraftmetascript_parser.gois excluded by!lang/grammar/*
📒 Files selected for processing (107)
.gitignore(1 hunks).helix/config.toml(1 hunks).helix/languages.toml(1 hunks).repomixignore(1 hunks).windsurfrules(1 hunks)cmd/build.go(4 hunks)cmd/lint.go(1 hunks)cmd/lsp.go(2 hunks)cmd/spec.go(1 hunks)examples/overworld-noise-settings/surface-rules/badlands.mms(3 hunks)examples/test_files/simple_noise.mms(1 hunks)examples/test_files/surface_rules.mms(1 hunks)flake.nix(2 hunks)go.mod(2 hunks)grammar/BlockState.g4(0 hunks)grammar/Core_Lang.g4(0 hunks)grammar/DensityFunctions.g4(0 hunks)grammar/Func.g4(0 hunks)grammar/MinecraftMetascript.g4(1 hunks)grammar/Noise.g4(0 hunks)grammar/NoiseRouter.g4(0 hunks)grammar/NoiseSettings.g4(0 hunks)grammar/Surface.g4(0 hunks)lang/DensityFn.go(1 hunks)lang/Dimensions.go(1 hunks)lang/File.go(0 hunks)lang/Noise.go(1 hunks)lang/Project.go(0 hunks)lang/Registries.go(1 hunks)lang/SharedBuilders.go(1 hunks)lang/SurfaceRules.go(1 hunks)lang/ast/Diagnostic.go(1 hunks)lang/ast/DiagnosticErrorListener.go(2 hunks)lang/ast/Locations.go(1 hunks)lang/ast/Namespace.go(1 hunks)lang/ast/Node.go(1 hunks)lang/ast/Symbol.go(1 hunks)lang/builder_chain/BuilderChain.go(0 hunks)lang/builder_chain/SharedBuilder.go(0 hunks)lang/constructs/block_states/BlockState.go(0 hunks)lang/constructs/mmsFunc/Registry.go(0 hunks)lang/constructs/mmsFunc/Signature.go(0 hunks)lang/constructs/primitives/VerticalAnchor.go(0 hunks)lang/constructs/worldgen/density_functions/ClampFn.go(0 hunks)lang/constructs/worldgen/density_functions/ConstantFn.go(0 hunks)lang/constructs/worldgen/density_functions/DensityFn.go(0 hunks)lang/constructs/worldgen/density_functions/DensityFnFunc.go.bak(0 hunks)lang/constructs/worldgen/density_functions/DualInput.go(0 hunks)lang/constructs/worldgen/density_functions/FunctionRef.go(0 hunks)lang/constructs/worldgen/density_functions/NoiseFn.go(0 hunks)lang/constructs/worldgen/density_functions/OldNoiseFn.go(0 hunks)lang/constructs/worldgen/density_functions/RangeChoiceFn.go(0 hunks)lang/constructs/worldgen/density_functions/ShiftedNoiseFn.go(0 hunks)lang/constructs/worldgen/density_functions/SingleInput.go(0 hunks)lang/constructs/worldgen/density_functions/SplineFn.go(0 hunks)lang/constructs/worldgen/density_functions/WeirdScaledSamplerFn.go(0 hunks)lang/constructs/worldgen/density_functions/YClampedGradientFn.go(0 hunks)lang/constructs/worldgen/noise/Noise.go(0 hunks)lang/constructs/worldgen/noise_router/NoiseRouter.go(0 hunks)lang/constructs/worldgen/noise_settings/NoiseSettings.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_AboveSurface.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_AboveWater.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Biome.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Compound_And.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Compound_Or.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Freezing.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Hole.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Noise.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Not.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Reference.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_Steep.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go(0 hunks)lang/constructs/worldgen/surface_rules/Condition_YAbove.go(0 hunks)lang/constructs/worldgen/surface_rules/Rule_Bandlands.go(0 hunks)lang/constructs/worldgen/surface_rules/Rule_Block.go(0 hunks)lang/constructs/worldgen/surface_rules/Rule_If.go(0 hunks)lang/constructs/worldgen/surface_rules/Rule_Reference.go(0 hunks)lang/constructs/worldgen/surface_rules/Rule_Sequence.go(0 hunks)lang/constructs/worldgen/surface_rules/SurfaceConditions.go(0 hunks)lang/constructs/worldgen/surface_rules/SurfaceRules.go(0 hunks)lang/cross_namespace_test.go(0 hunks)lang/diagnostics_file_test.go(0 hunks)lang/diagnostics_namespace_test.go(0 hunks)lang/duplicates_test.go(0 hunks)lang/parsing_and_export_test.go(0 hunks)lang/purge_reparse_test.go(0 hunks)lang/spec/Block.go(1 hunks)lang/spec/Condition.go(1 hunks)lang/spec/Enum.go(1 hunks)lang/spec/Function.go(1 hunks)lang/spec/Generation.go(1 hunks)lang/spec/List.go(1 hunks)lang/spec/Number.go(1 hunks)lang/spec/Reference.go(1 hunks)lang/spec/String.go(1 hunks)lang/spec/Tag.go(1 hunks)lang/spec/Value.go(1 hunks)lang/surface_rules_additional_test.go(0 hunks)lang/syntax_errors_test.go(0 hunks)lang/traversal/Construct.go(0 hunks)lang/traversal/Location.go(0 hunks)lang/traversal/Parser.go(0 hunks)lang/traversal/Reference.go(0 hunks)lang/traversal/Scope.go(0 hunks)lang/traversal/Symbol.go(0 hunks)lang/unpack/Args.go(1 hunks)
⛔ Files not processed due to max files limit (40)
- lang/unpack/Assignment.go
- lang/unpack/Builders.go
- lib/FileTreeLike.go
- lib/IsNilInterface.go
- lib/Unique.go
- lsp/FoldingRangeVisitor.go
- lsp/LanguageServer.go
- lsp/PublishFileDiagnostics.go
- lsp/Symbols.go
- lsp/document.go
- main_wasm.go
- project/File.go
- project/Parser.go
- project/Project.go
- repomix.config.json
- test.mms
- vendor/github.com/samber/lo/.gitignore
- vendor/github.com/samber/lo/Dockerfile
- vendor/github.com/samber/lo/LICENSE
- vendor/github.com/samber/lo/Makefile
- vendor/github.com/samber/lo/channel.go
- vendor/github.com/samber/lo/concurrency.go
- vendor/github.com/samber/lo/condition.go
- vendor/github.com/samber/lo/constraints.go
- vendor/github.com/samber/lo/errors.go
- vendor/github.com/samber/lo/find.go
- vendor/github.com/samber/lo/func.go
- vendor/github.com/samber/lo/internal/constraints/constraints.go
- vendor/github.com/samber/lo/internal/constraints/ordered_go118.go
- vendor/github.com/samber/lo/internal/constraints/ordered_go121.go
- vendor/github.com/samber/lo/internal/rand/ordered_go118.go
- vendor/github.com/samber/lo/internal/rand/ordered_go122.go
- vendor/github.com/samber/lo/intersect.go
- vendor/github.com/samber/lo/map.go
- vendor/github.com/samber/lo/math.go
- vendor/github.com/samber/lo/mutable/slice.go
- vendor/github.com/samber/lo/retry.go
- vendor/github.com/samber/lo/slice.go
- vendor/github.com/samber/lo/string.go
- vendor/github.com/samber/lo/time.go
💤 Files with no reviewable changes (68)
- lang/constructs/worldgen/surface_rules/Rule_Bandlands.go
- lang/duplicates_test.go
- lang/cross_namespace_test.go
- lang/diagnostics_file_test.go
- lang/parsing_and_export_test.go
- lang/constructs/worldgen/surface_rules/Condition_Hole.go
- lang/constructs/mmsFunc/Registry.go
- grammar/Core_Lang.g4
- lang/constructs/worldgen/surface_rules/Condition_Reference.go
- lang/constructs/block_states/BlockState.go
- lang/constructs/worldgen/surface_rules/Condition_Steep.go
- lang/File.go
- lang/constructs/worldgen/surface_rules/Condition_Compound_Or.go
- lang/constructs/worldgen/density_functions/NoiseFn.go
- lang/constructs/worldgen/density_functions/FunctionRef.go
- lang/constructs/worldgen/density_functions/RangeChoiceFn.go
- lang/constructs/worldgen/noise/Noise.go
- lang/traversal/Reference.go
- lang/traversal/Location.go
- lang/constructs/worldgen/surface_rules/Condition_AboveSurface.go
- lang/constructs/worldgen/surface_rules/Condition_Compound_And.go
- grammar/Noise.g4
- lang/constructs/worldgen/surface_rules/Condition_Freezing.go
- lang/constructs/primitives/VerticalAnchor.go
- lang/constructs/worldgen/surface_rules/Condition_AboveWater.go
- lang/constructs/worldgen/density_functions/SplineFn.go
- lang/constructs/worldgen/density_functions/SingleInput.go
- lang/constructs/worldgen/density_functions/ShiftedNoiseFn.go
- lang/purge_reparse_test.go
- lang/traversal/Construct.go
- lang/constructs/worldgen/density_functions/DensityFn.go
- grammar/Func.g4
- grammar/Surface.g4
- lang/constructs/worldgen/surface_rules/SurfaceRules.go
- grammar/NoiseSettings.g4
- lang/constructs/worldgen/surface_rules/Rule_If.go
- lang/constructs/mmsFunc/Signature.go
- lang/constructs/worldgen/density_functions/ConstantFn.go
- grammar/DensityFunctions.g4
- lang/builder_chain/SharedBuilder.go
- lang/constructs/worldgen/surface_rules/Rule_Reference.go
- lang/constructs/worldgen/noise_settings/NoiseSettings.go
- lang/builder_chain/BuilderChain.go
- lang/constructs/worldgen/density_functions/DensityFnFunc.go.bak
- grammar/BlockState.g4
- lang/constructs/worldgen/density_functions/ClampFn.go
- lang/diagnostics_namespace_test.go
- grammar/NoiseRouter.g4
- lang/constructs/worldgen/surface_rules/Condition_YAbove.go
- lang/constructs/worldgen/surface_rules/Rule_Sequence.go
- lang/constructs/worldgen/noise_router/NoiseRouter.go
- lang/constructs/worldgen/density_functions/YClampedGradientFn.go
- lang/constructs/worldgen/surface_rules/Condition_Not.go
- lang/constructs/worldgen/surface_rules/Rule_Block.go
- lang/surface_rules_additional_test.go
- lang/constructs/worldgen/density_functions/WeirdScaledSamplerFn.go
- lang/syntax_errors_test.go
- lang/Project.go
- lang/constructs/worldgen/surface_rules/Condition_VerticalGradient.go
- lang/constructs/worldgen/density_functions/DualInput.go
- lang/constructs/worldgen/surface_rules/Condition_Noise.go
- lang/traversal/Symbol.go
- lang/constructs/worldgen/density_functions/OldNoiseFn.go
- lang/traversal/Scope.go
- lang/constructs/worldgen/surface_rules/SurfaceConditions.go
- lang/constructs/worldgen/surface_rules/Condition_Biome.go
- lang/constructs/worldgen/surface_rules/Condition_StoneDepth.go
- lang/traversal/Parser.go
🧰 Additional context used
🧬 Code graph analysis (27)
cmd/lint.go (3)
lang/purge_reparse_test.go (1)
TestPurgeSymbolsOnReparse_RemovesOldExports(7-37)lang/Project.go (1)
p(21-29)lang/diagnostics_namespace_test.go (1)
TestMissingSemicolonInNamespace_ReportsSyntaxErrorWithLocation(8-35)
lang/ast/Diagnostic.go (5)
lang/ast/Locations.go (2)
Location(11-18)SourceLocation(54-58)lang/traversal/Diagnostics.go (5)
l(53-90)Message(22-28)DefaultErrorListener(37-42)d(30-32)NewDiagnosticsErrorListener(44-50)lang/traversal/Scope.go (2)
s(76-87)s(89-101)lang/Project.go (1)
p(21-29)lang/diagnostics_namespace_test.go (1)
TestMissingSemicolonInNamespace_ReportsSyntaxErrorWithLocation(8-35)
lang/ast/Namespace.go (3)
lang/ast/Symbol.go (2)
Symbol(48-63)MarshalSymbol(137-145)lang/ast/Diagnostic.go (2)
Diagnostic(23-27)Error(6-6)lang/ast/Locations.go (1)
Location(11-18)
lang/unpack/Args.go (2)
lang/ast/Node.go (1)
Node(7-10)lib/IsNilInterface.go (1)
IsNilInterface(5-11)
lang/spec/Tag.go (5)
lang/ast/Symbol.go (2)
TagKind(11-11)BaseSymbol(65-72)lang/grammar/minecraftmetascript_parser.go (1)
IValueContext(1926-1943)lang/ast/Node.go (2)
Node(7-10)BaseNode(12-14)lang/ast/Diagnostic.go (2)
Diagnostic(23-27)Warning(7-7)lang/ast/Locations.go (2)
Location(11-18)RuleLocation(130-135)
lang/spec/Number.go (5)
lang/ast/Symbol.go (2)
SymbolKind(34-34)BaseSymbol(65-72)lang/grammar/minecraftmetascript_parser.go (1)
IValueContext(1926-1943)lang/ast/Node.go (2)
Node(7-10)BaseNode(12-14)lang/ast/Diagnostic.go (2)
Diagnostic(23-27)Warning(7-7)lang/ast/Locations.go (2)
RuleLocation(130-135)Location(11-18)
cmd/spec.go (2)
lang/spec/Generation.go (1)
GenerateSpecString(10-30)lang/Registries.go (1)
Blocks(7-7)
lang/spec/Generation.go (8)
lang/spec/Block.go (1)
BlockSpecList(14-16)lang/spec/Value.go (2)
ValueSpec(44-47)ValueSpecList(49-52)lang/spec/Function.go (1)
FunctionSpec(170-180)lang/spec/Number.go (1)
NumberSpec(10-13)lang/spec/Reference.go (1)
ReferenceSpec(15-18)lang/spec/List.go (1)
ListSpec(15-22)lang/spec/Condition.go (1)
ConditionalSpec(15-22)lang/ast/Node.go (1)
Helpful(24-26)
lang/SharedBuilders.go (6)
lang/spec/Function.go (3)
NewOverloadSpec(64-70)NewFunctionSpec(466-471)FunctionNode(473-484)lang/spec/Value.go (2)
ValueSpec(44-47)NewValueSpecList(118-125)lang/spec/Number.go (2)
NewNumberSpec(23-27)GetNumberNodeValue(78-86)lang/DensityFn.go (1)
DensityFunctions(112-112)lang/unpack/Assignment.go (1)
RegisterParser(16-18)lang/ast/Node.go (1)
Node(7-10)
cmd/build.go (1)
project/Project.go (1)
NewProject(17-22)
lang/spec/String.go (4)
lang/grammar/minecraftmetascript_parser.go (1)
IValueContext(1926-1943)lang/ast/Node.go (2)
Node(7-10)BaseNode(12-14)lang/ast/Diagnostic.go (1)
Diagnostic(23-27)lang/ast/Locations.go (2)
TerminalLocation(137-142)Location(11-18)
lang/spec/List.go (7)
lang/spec/Value.go (3)
ValueSpec(44-47)MethodKind(15-15)SnippetFormat(14-14)lang/ast/Symbol.go (2)
SymbolKind(34-34)BaseSymbol(65-72)lib/IsNilInterface.go (1)
IsNilInterface(5-11)lang/grammar/minecraftmetascript_parser.go (1)
IValueContext(1926-1943)lang/ast/Node.go (2)
Node(7-10)CompletableNode(32-34)lang/ast/Diagnostic.go (2)
Diagnostic(23-27)Error(6-6)lang/ast/Locations.go (2)
RuleLocation(130-135)Location(11-18)
lang/spec/Reference.go (8)
lang/ast/Symbol.go (4)
SymbolKind(34-34)SymbolNever(37-37)BaseSymbol(65-72)Symbol(48-63)lang/ast/Namespace.go (1)
Namespace(8-11)lang/spec/Value.go (2)
ExtractPrefixAtPosition(23-42)ReferenceKind(16-16)lang/grammar/minecraftmetascript_parser.go (1)
IValueContext(1926-1943)lang/ast/Node.go (2)
Node(7-10)BaseNode(12-14)lang/ast/Diagnostic.go (2)
Diagnostic(23-27)Warning(7-7)lang/ast/Locations.go (2)
Location(11-18)RuleLocation(130-135)lang/spec/Tag.go (1)
TagNode(130-135)
lang/spec/Enum.go (7)
lang/spec/Value.go (2)
ExtractPrefixAtPosition(23-42)EnumKind(18-18)lang/grammar/minecraftmetascript_parser.go (1)
IValueContext(1926-1943)lang/ast/Node.go (2)
Node(7-10)BaseNode(12-14)lang/ast/Diagnostic.go (2)
Diagnostic(23-27)Error(6-6)lang/ast/Locations.go (2)
RuleLocation(130-135)Location(11-18)lang/ast/Symbol.go (1)
BaseSymbol(65-72)vendor/github.com/samber/lo/find.go (1)
IndexOf(13-21)
lang/spec/Block.go (8)
lang/spec/Value.go (5)
ValueSpecList(49-52)ValueSpec(44-47)NewValueSpecList(118-125)StructKind(17-17)SnippetFormat(14-14)lang/ast/Symbol.go (3)
Symbol(48-63)EmptySymbol(74-76)BaseSymbol(65-72)lang/grammar/minecraftmetascript_parser.go (1)
IBlockContext(790-805)lang/ast/Diagnostic.go (2)
Diagnostic(23-27)Error(6-6)lang/ast/Locations.go (4)
Location(11-18)RuleLocation(130-135)TerminalLocation(137-142)SourceLocation(54-58)lib/IsNilInterface.go (1)
IsNilInterface(5-11)lang/ast/Node.go (4)
BaseNode(12-14)ExtractableNode(36-38)CompletableNode(32-34)Node(7-10)lang/spec/List.go (1)
ListSpec(15-22)
cmd/lsp.go (1)
lsp/LanguageServer.go (1)
Start(72-83)
lang/ast/Symbol.go (3)
lang/ast/Node.go (2)
Node(7-10)BaseNode(12-14)wasm/mms.d.ts (1)
FileTreeLike(24-27)lang/ast/Locations.go (2)
SourceLocation(54-58)Location(11-18)
lang/Registries.go (1)
lang/spec/Block.go (1)
NewBlockSpecList(33-43)
lang/spec/Condition.go (9)
lang/spec/Value.go (3)
ValueSpec(44-47)MethodKind(15-15)SnippetFormat(14-14)lang/ast/Symbol.go (3)
SymbolKind(34-34)BaseSymbol(65-72)Symbol(48-63)lang/ast/Namespace.go (1)
Namespace(8-11)lang/ast/Node.go (3)
Node(7-10)BaseNode(12-14)CompletableNode(32-34)lang/grammar/minecraftmetascript_parser.go (7)
IConditionContext(2178-2185)CondAndContext(2331-2333)CondOrContext(2410-2412)CondGroupedContext(2231-2233)CondNegateContext(2285-2287)CondPrimaryContext(2489-2491)IValueContext(1926-1943)lang/ast/Diagnostic.go (3)
Diagnostic(23-27)Error(6-6)Warning(7-7)lang/ast/Locations.go (3)
RuleLocation(130-135)Location(11-18)SourceLocation(54-58)lib/IsNilInterface.go (1)
IsNilInterface(5-11)lang/SurfaceRules.go (1)
Conditional(409-409)
lang/Dimensions.go (5)
lang/spec/Function.go (4)
FunctionNode(473-484)NewFunctionSpec(466-471)NewOverloadSpec(64-70)FunctionSpec(170-180)lib/FileTreeLike.go (1)
NewDirLike(125-132)lang/unpack/Builders.go (1)
Builders(13-71)lang/SharedBuilders.go (3)
EmptyFn(11-11)SimpleIntFn(19-25)SimpleFloatFn(12-18)lang/ast/Symbol.go (7)
TagBlock(14-14)SymbolDimensionType(39-39)SymbolDimension(42-42)SymbolNever(37-37)SymbolNoiseRouter(40-40)SymbolSurfaceRule(43-43)SymbolNoiseSettings(41-41)
lang/spec/Value.go (5)
lang/grammar/minecraftmetascript_parser.go (1)
IValueContext(1926-1943)lang/ast/Node.go (2)
Node(7-10)CompletableNode(32-34)lang/ast/Diagnostic.go (1)
Diagnostic(23-27)lang/ast/Namespace.go (1)
Namespace(8-11)lib/IsNilInterface.go (1)
IsNilInterface(5-11)
lang/spec/Function.go (8)
lang/spec/Value.go (3)
ValueSpec(44-47)MethodKind(15-15)SnippetFormat(14-14)lang/unpack/Builders.go (1)
Builders(13-71)lang/ast/Node.go (3)
BaseNode(12-14)Node(7-10)CompletableNode(32-34)lang/grammar/minecraftmetascript_parser.go (2)
IFnContext(1500-1514)IValueContext(1926-1943)lang/ast/Diagnostic.go (3)
Diagnostic(23-27)Error(6-6)Warning(7-7)lang/ast/Locations.go (3)
RuleLocation(130-135)Location(11-18)SourceLocation(54-58)lang/ast/Symbol.go (3)
SymbolKind(34-34)Symbol(48-63)BaseSymbol(65-72)lib/FileTreeLike.go (1)
FileTreeLike(9-15)
lang/ast/Node.go (3)
lang/ast/Locations.go (2)
SourceLocation(54-58)Location(11-18)lang/ast/Namespace.go (1)
Namespace(8-11)lang/ast/Symbol.go (1)
Symbol(48-63)
lang/Noise.go (9)
lang/spec/Function.go (4)
NewFunctionSpec(466-471)NewOverloadSpec(64-70)FunctionSpec(170-180)FunctionNode(473-484)lang/spec/Value.go (2)
ValueSpec(44-47)NewValueSpecList(118-125)lang/spec/Number.go (2)
NewNumberSpec(23-27)NumberNode(72-76)lang/ast/Symbol.go (2)
SymbolNoise(38-38)Symbol(48-63)lang/spec/Block.go (1)
NewBlockSpec(56-58)lib/FileTreeLike.go (2)
FileTreeLike(9-15)NewDirLike(125-132)lang/spec/Reference.go (2)
NewReferenceSpec(28-30)GetReferenceNodeValue(157-169)lang/unpack/Assignment.go (1)
RegisterParser(16-18)lib/IsNilInterface.go (1)
IsNilInterface(5-11)
lang/DensityFn.go (11)
lang/spec/Function.go (4)
FunctionNode(473-484)FunctionSpec(170-180)NewFunctionSpec(466-471)NewOverloadSpec(64-70)lib/FileTreeLike.go (1)
NewDirLike(125-132)lang/ast/Symbol.go (2)
Symbol(48-63)SymbolDensityFunction(45-45)lang/spec/Enum.go (2)
NewEnumSpec(60-62)GetEnumNodeValue(160-168)lang/Noise.go (3)
GetInlinedNoiseRef(98-108)InlinedNoise(92-92)ExtractInlineNoiseSymbol(110-122)lang/SharedBuilders.go (10)
XzScale(31-31)YScale(32-32)XzFactor(33-33)YFactor(34-34)SmearScaleMul(35-35)ShiftX(27-27)ShiftY(28-28)ShiftZ(29-29)MinBuilder(36-36)MaxBuilder(37-37)lang/unpack/Builders.go (1)
Builders(13-71)lang/spec/Number.go (2)
GetNumberNodeValue(78-86)NumberNode(72-76)lib/IsNilInterface.go (1)
IsNilInterface(5-11)lang/SurfaceRules.go (1)
SimpleSerializer(37-46)lang/unpack/Args.go (1)
Args(26-55)
lang/ast/DiagnosticErrorListener.go (2)
lang/ast/Diagnostic.go (3)
Diagnostics(29-31)Diagnostic(23-27)Error(6-6)lang/ast/Locations.go (2)
SourceLocation(54-58)Location(11-18)
lang/SurfaceRules.go (9)
lang/spec/Function.go (4)
FunctionNode(473-484)NewFunctionSpec(466-471)NewOverloadSpec(64-70)FunctionSpec(170-180)lib/FileTreeLike.go (1)
NewDirLike(125-132)lang/SharedBuilders.go (6)
SimpleFloatFn(12-18)EmptyFn(11-11)MinBuilder(36-36)MaxBuilder(37-37)OffsetBuilder(38-38)VerticalAnchor(40-43)lang/ast/Symbol.go (5)
SymbolSurfaceRule(43-43)SymbolNever(37-37)SymbolSurfaceCondition(44-44)SymbolNoise(38-38)Symbol(48-63)lang/spec/Reference.go (2)
NewReferenceSpec(28-30)GetReferenceNodeValue(157-169)lang/unpack/Builders.go (1)
Builders(13-71)lang/unpack/Args.go (1)
Args(26-55)lang/spec/Enum.go (2)
NewEnumSpec(60-62)GetEnumNodeValue(160-168)lang/spec/Condition.go (6)
NewConditionSpec(28-33)ConditionalNode(250-257)ConditionalSpec(15-22)ConditionAndNode(83-87)ConditionOrNode(93-97)ConditionNegateNode(103-106)
🔇 Additional comments (25)
examples/overworld-noise-settings/surface-rules/badlands.mms (3)
16-18: LGTM! Cleaner syntax with explicit operators.The refactor from
Or(...)wrapper to explicit||operators improves readability while maintaining equivalent logic for the three noise threshold ranges.
44-48: Clarify the behavior ofAdd()without arguments.The condition uses
!YAbove(74).Mul(1).Add()whereAdd()is called without arguments. This pattern appears in multiple places (lines 24, 46, 60), but the behavior is unclear—what value is being added when no argument is provided?Please clarify the intended behavior of
Add()when called without arguments, or consider documenting this API usage to improve code clarity.
1-1: Namespace declarations are consistently capitalized. No lowercasenamespacekeywords remain..gitignore (1)
10-11: LGTM!The addition of these ignore patterns is appropriate for excluding generated output files and specification documents from version control.
.helix/languages.toml (1)
10-10: LGTM!The comment token change from
#to//aligns with the broader grammar and syntax updates in the PR.cmd/lint.go (1)
17-51: Clarify the intent of disabling the lint command.The entire lint implementation is commented out, leaving a no-op command. This could confuse users who invoke
mms lintexpecting it to perform linting.Please clarify:
- Is this a temporary measure during the refactor?
- Should the command be removed entirely until re-implemented?
- Should it return an error message indicating the feature is unavailable?
Consider either removing the command registration or adding a helpful error message to inform users that linting is not yet available in the new architecture.
cmd/build.go (2)
73-84: LGTM with note on serialization changes.The serialization logic has been updated to use
project.Symbols()instead of the previous approach. The error handling is appropriate, with debug mode providing verbose output and non-debug mode still performing serialization for validation purposes.
59-64: Confirm parsing is handled in AddFile
AddFile invokes f.Parse() internally (see project/Project.go:45), so no explicit Parse() call is required.go.mod (2)
7-7: lo v1.51.0 is a valid release and has no known vulnerabilities.
34-34: golang.org/x/text v0.29.0 verified
v0.29.0 is published and has no known security advisories.lang/Registries.go (1)
1-7: LGTM!The file cleanly establishes the central Blocks registry for the language. The initialization is straightforward and follows Go conventions for package-level variable declarations.
cmd/spec.go (1)
14-29: LGTM!The command implementation is clean and well-documented. The integration with
spec.GenerateSpecString(lang.Blocks)correctly leverages the spec generation framework, and the command registration follows standard Cobra patterns.lang/spec/String.go (2)
38-46: LGTM!The helper function includes proper nil checks and safe type assertions, following Go best practices for optional value extraction.
20-31: Use precise quote removal and add edge-case tests
- Replace
strings.Trim(val.GetText(), "\"")withstrings.TrimSuffix(strings.TrimPrefix(val.GetText(), "\""), "\"")or slice the literal (text[1:len(text)-1]) to remove exactly one quote per side.- No tests found for inputs with embedded quotes—add cases (e.g.,
"\"hello\"") to verify trimming behavior.lang/spec/Generation.go (1)
10-30: LGTM!The function cleanly generates a structured markdown specification from the block registry. The iteration logic and formatting are appropriate for the use case.
lang/spec/Tag.go (2)
66-128: LGTM!The validation logic comprehensively checks for:
- Missing tag references
- Invalid prefixes/suffixes (leading/trailing colons)
- Single-part and two-part identifier parsing
- Proper error diagnostics for invalid cases
The implementation correctly handles namespace defaults and constructs appropriate AST nodes.
147-159: LGTM!The helper function includes proper nil checks, type assertions, and kind validation, ensuring type-safe value extraction.
lang/ast/Node.go (1)
7-38: LGTM!The AST node framework is well-designed with:
- Clean interface separation (Node, Helpful, CompletableNode, ExtractableNode)
- Sensible default implementations in BaseNode
- Proper composition patterns (HelpfulNode combining Node and Helpful)
The interfaces provide good extension points for various node capabilities without forcing all nodes to implement every feature.
lang/SharedBuilders.go (1)
48-77: LGTM!The ParseAnchor function correctly handles three cases:
- Positive numbers → "above_bottom"
- Negative numbers → "below_top" (with absolute value)
- Abs function → "absolute"
The logic includes proper nil checks and type assertions. The use of
math.Absfor negative values is appropriate.lang/ast/Namespace.go (1)
8-67: LGTM overall!The Namespace implementation provides clean encapsulation of symbol management with:
- Proper initialization via constructor
- Safe map operations
- Duplicate detection in Declare
- Custom JSON serialization via MarshalSymbol
The structure follows Go best practices for managing collections and provides a solid foundation for namespace handling.
lang/SurfaceRules.go (5)
330-348: DepthMultiplier default nil panic risk
YAboveSerializerusesunpack.Builders. If builders missing,DepthMultiplierfield stays zero. No issue. No comment.
354-387: Conditional init lacks exporter registration
Conditionalspec defines output but not file exporter. If other blocks rely on exporting surface conditionals, missing exporter might break file generation. Confirm whetherSetFileExporteris required. If so, add. (Need evidence) Possibly not. Skip.
139-141: ...Then Spline. Then duplicate. Let's go. ☴. (Remember to ensure comment text trimmed, direct, not explanation heavy.) Construct spool. Also final summary? At end we must produce summary/outstanding. But before final output ensure all instructions satisfied. Need to maintain scratchpad etc. Continue. Also need to mark scratchpad tasks done. After comments, final summary referencing major issues. Let's produce new comments. We need to embed them within file block. Add new comments. Append to file. Also ensure recognized whether comment referencing same lines as earlier? fine. Let's craft new comments. Need to ensures disclaim? abiding illusions. Ok. Add:
139-141: Check slice length before indexing
...But note we previously flagged pointer nil guard same lines. We'll mention new issue. For fix snippet, we can propose reorder. Now Spline comment:
440-465: Spline points never capture argument data
...Need to articulate. Third comment (duplicate) referencing 235? that'll ack. Let's craft. Also check comment referencing lexical. Let's produce text. Need to ensure instructions: keep bullet? not necessary. Ok. Also update scratchpad to mark done. Finally produce final summary referencing issues. Also mention tests? Should we request additional tests verifying? maybe we can note. Let's craft final. Need to rectify scratchpad: mark tasks done. Let's finalize. We'll produce final output. Ensure final summary includes major issues: `Cache` nil panic, `unary` nil pointer, `getDensityFn` guard, Spline missing tags, etc. Provide verifying instructions maybe. Also include `SurfaceRules` final. In final summary mention invalid comment? Maybe not. Ok final message. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| //fsLike := project.BuildFsLike(outFile) | ||
| if err != nil { | ||
| log.Println("Error exporting project:", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Remove dead code checking undefined error.
The BuildFsLike call is commented out (Line 66), but Lines 67-70 still check err which is never set, making this dead code that always evaluates to false.
Apply this diff to remove the dead code:
- //fsLike := project.BuildFsLike(outFile)
- if err != nil {
- log.Println("Error exporting project:", err)
- return
- }If the export functionality is intentionally disabled, consider adding a TODO comment explaining when it will be re-enabled.
📝 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.
| //fsLike := project.BuildFsLike(outFile) | |
| if err != nil { | |
| log.Println("Error exporting project:", err) | |
| return | |
| } |
🤖 Prompt for AI Agents
In cmd/build.go around lines 66 to 70, the code checks an undefined err from a
commented-out BuildFsLike call, leaving dead code; remove the unused err check
block (lines 67–70) and either delete the commented-out BuildFsLike line too or
add a brief TODO comment above it explaining why export is disabled and when it
will be re-enabled; ensure no references to err remain and that the function
compiles after removal.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/build.go (1)
69-73: Remove dead code checking undefined error.Line 69 calls
BuildFsLikewhich returns*lib.FileTreeLike(not an error), but lines 70-73 check anerrvariable that was last set on Line 64. This is dead code that will never execute.Apply this diff to remove the dead code:
fsLike := project.BuildFsLike(outFile) - if err != nil { - log.Println("Error exporting project:", err) - return - }
🧹 Nitpick comments (4)
cmd/lsp.go (1)
17-19: Error handling implemented correctly.The error from
lsp.Start()is now properly captured and handled withlog.Fatalf, addressing the previous review comment.Minor nitpick: The
\nin the format string is redundant aslog.Fatalfautomatically adds a newline.If desired, you can remove the redundant newline:
- log.Fatalf("Failed to start LSP server: %v\n", err) + log.Fatalf("Failed to start LSP server: %v", err)lang/unpack/Assignment.go (2)
30-33: Address the TODO for proper boolean value extraction.The
assignBoolfunction currently always sets the field totrue, ignoring the input node. The TODO comment indicates thatspec.GetBooleanNodeValueneeds to be implemented.Do you want me to help implement
spec.GetBooleanNodeValueor open a new issue to track this task?
35-39: Remove commented-out dead code.The
assignSymbolfunction is commented out. If this functionality is not needed, remove the commented code. If it will be needed later, create a TODO comment explaining when it will be re-enabled, or track it in an issue.-//func assignSymbol(arg ast.Node, mutableField reflect.Value, kind ast.SymbolKind) { -// if val := spec.GetReferenceNodeValue(arg, kind); val != nil { -// mutableField.Set(reflect.ValueOf(*val)) -// } -//}cmd/build.go (1)
128-141: Prefer os.IsNotExist() over string matching.The function uses fragile error message string matching. Use
os.IsNotExist(err)instead for more robust error checking.Apply this diff:
func mkdirIfNotExists(dirPath string) error { _, err := fs.Stat(os.DirFS("."), dirPath) if err != nil { - if strings.HasSuffix(err.Error(), "no such file or directory") { + if os.IsNotExist(err) { err = os.MkdirAll(dirPath, 0755) if err != nil { return err } } else { return err } } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)cmd/build.go(4 hunks)cmd/lint.go(1 hunks)cmd/lsp.go(2 hunks)cmd/spec.go(1 hunks)examples/test_files/surface_rules.mms(1 hunks)lang/unpack/Assignment.go(1 hunks)lang/unpack/Builders.go(1 hunks)lib/FileTreeLike.go(2 hunks)project/Project.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/lint.go
- examples/test_files/surface_rules.mms
🧰 Additional context used
🧬 Code graph analysis (6)
cmd/lsp.go (1)
lsp/LanguageServer.go (1)
Start(72-83)
cmd/build.go (2)
project/Project.go (1)
NewProject(17-22)lib/FileTreeLike.go (1)
FileTreeLike(9-15)
lang/unpack/Assignment.go (6)
lang/ast/Node.go (1)
Node(7-10)lang/spec/Number.go (1)
GetNumberNodeValue(78-86)lang/spec/String.go (1)
GetStringNodeValue(38-46)lang/ast/Symbol.go (1)
Symbol(48-63)lib/IsNilInterface.go (1)
IsNilInterface(5-11)lang/spec/Function.go (1)
FunctionNode(473-484)
cmd/spec.go (2)
lang/spec/Generation.go (1)
GenerateSpecString(10-30)lang/Registries.go (1)
Blocks(7-7)
project/Project.go (9)
project/File.go (1)
File(12-17)lang/ast/Namespace.go (1)
Namespace(8-11)lang/ast/Locations.go (1)
SourceLocation(54-58)lang/ast/Node.go (1)
Node(7-10)lib/FileTreeLike.go (2)
FileTreeLike(9-15)NewDirLike(140-147)wasm/mms.d.ts (1)
FileTreeLike(24-27)lang/ast/Diagnostic.go (2)
Diagnostic(23-27)Diagnostics(29-31)vendor/github.com/samber/lo/slice.go (1)
Reduce(86-92)vendor/github.com/samber/lo/map.go (1)
Values(54-68)
lang/unpack/Builders.go (2)
lang/spec/Function.go (1)
FunctionNode(473-484)vendor/github.com/samber/lo/find.go (1)
Find(38-47)
🔇 Additional comments (12)
cmd/spec.go (2)
12-22: LGTM!The spec command implementation is clean and straightforward. It correctly integrates with the spec generation system to output the markdown-formatted language specification.
24-26: LGTM!The command registration follows the standard Cobra pattern.
lib/FileTreeLike.go (3)
17-27: LGTM! Recursive merge implementation is correct.The
Mergemethod correctly handles recursive merging of directory trees by:
- Only operating when both nodes are directories
- Adding new children that don't exist
- Recursively merging children that exist in both trees
95-101: LGTM!The
HasChildmethod is a straightforward and correct helper that safely checks for child existence with proper nil handling.
103-114: Consider handling or documenting the dir/file conflict case.When
HasChildreturns true but the existing and new children have mismatched types (one is a directory, the other is a file), the code silently does nothing. This might be intentional (preserve existing), but it could lead to confusion.Consider one of these approaches:
- Document this behavior in a comment
- Log a warning when a conflict occurs
- Return an error to make conflicts explicit
For example:
func (ft *FileTreeLike) AddChild(child *FileTreeLike) { if ft.IsDir { if ft.HasChild(child.Name) { if ft.Children[child.Name].IsDir && child.IsDir { ft.Children[child.Name].Merge(child) return + } else { + // Type mismatch: existing and new child have different types + // Preserving existing child, ignoring new one + return } } else { ft.Children[child.Name] = child } } }lang/unpack/Builders.go (1)
12-69: LGTM! Complex but correct reflection-based builder logic.The
Buildersfunction uses reflection to dynamically populate struct fields based onmms_buildertags. The logic correctly handles:
- Finding matching builders by name
- Parsing and validating argument indices
- Flag-style builders (no arguments)
- Regular builders with arguments
- Skipping invalid or missing builders
The implementation is complex but appears sound.
lang/unpack/Assignment.go (2)
14-18: LGTM! Extensible parser registry pattern.The global
parseRegistrywith the publicRegisterParserfunction provides a clean extension point for custom type parsing.Note: The registry is not thread-safe. If parsers are registered concurrently (unlikely during typical initialization), consider adding synchronization.
75-97: Verify or clarify the incomplete mms_arg handling logic.In the symbol type handling, when
ToSerializable()returns nil and the node is aFunctionNode, the code checks themms_argtag and validates the argument index (lines 82-93), but then never usesargIdxto assign a value. This appears to be incomplete logic or dead code.Please clarify the intent:
- If this logic is incomplete and will be implemented later, add a TODO comment
- If this logic is meant to extract and assign a specific argument from the function, add the assignment logic
- If this code path should never be reached, add a log statement or remove it
For example, if you intend to assign the argument value:
if argIdx > len(fn.Arguments)-1 { break } // Assign the specific argument assignValue(field, fn.Arguments[argIdx], mutableField)cmd/build.go (1)
107-126: LGTM! Recursive file tree writing is correct.The
flushProjectfunction correctly handles recursive writing of the file tree structure, creating directories as needed and writing file contents with appropriate permissions.project/Project.go (3)
24-57: LGTM! Solid file management with declaration cleanup.The
AddFilemethod correctly handles file updates by:
- Removing old declarations from the file being replaced (lines 30-43)
- Parsing the new file content
- Merging declarations into the global symbol namespace
- Properly handling and returning parse errors
The nil-safety checks (line 33) and location-based cleanup (line 37) are good defensive programming.
59-73: LGTM! Clean file tree generation.The
BuildFsLikemethod correctly constructs a hierarchical file tree structure by organizing declarations under their respective namespaces. The nil check (line 67) before adding children is good defensive coding.
90-94: LGTM! Functional aggregation of diagnostics.The
AllDiagnosticsmethod elegantly useslo.Reduceandslices.Concatto aggregate diagnostics from all files. The functional approach is clean and concise.Based on learnings
1 participant
Resolves #6
Resolves #14
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores