feat(gnodoc): generate docs from contract codebase#1160
feat(gnodoc): generate docs from contract codebase#1160
Conversation
ValueSpec documentation
WalkthroughAdds GnoDoc: a CLI tool that parses Go/Gno sources into a DocPackage model (module-root detection, file discovery with include/exclude/test flags), builds anchors and indexes, and renders structured Markdown; includes a Runner CLI, Parser, Renderer, model types, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Parser
participant Model as DocPackage
participant Renderer
participant FS as FileSystem
User->>CLI: invoke gnodoc [cmd] [path] [flags]
CLI->>Parser: ParsePackage(path, options)
Parser->>Parser: detect module root (gnomod.toml) / resolve import path
Parser->>Parser: collect files (include/exclude, tests)
Parser->>Parser: parse files -> build symbols, examples, deprecations
Parser->>Model: return DocPackage
CLI->>Renderer: Render(DocPackage)
Renderer->>Renderer: register anchors, build sections
Renderer->>CLI: return Markdown
CLI->>FS: create/write output file(s)
CLI->>User: exit with ExitCode (Success/PartialError/Error)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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
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: 6
🧹 Nitpick comments (16)
gnodoc/.gitignore (1)
1-1: Clarify the ignore pattern to explicitly target the binary.This pattern will ignore anything named
gnodocwithin thegnodoc/directory. If the intent is to ignore the compiled binary, consider making it explicit:Suggested clarification
-gnodoc +# Compiled binary +/gnodocgnodoc/model/source_file_test.go (1)
44-75: Consider adding.gnotest file cases.Once
IsTestFile()is updated to support.gnofiles (as mentioned in parser options), add corresponding test cases:Suggested additional test cases
{ name: "gno regular file", file: SourceFile{Name: "foo.gno"}, expected: false, }, { name: "gno test file", file: SourceFile{Name: "foo_test.gno"}, expected: true, },gnodoc/cmd/options_test.go (1)
22-34: Consider testing embedded GlobalOptions defaults.The test validates ExportOptions-specific fields but doesn't verify that embedded
GlobalOptionsfields are also set correctly (e.g.,opts.ExportedOnly,opts.IncludeTests). This could catch regressions if the embedded defaults change.♻️ Suggested addition
func TestExportOptions_Default(t *testing.T) { opts := DefaultExportOptions() + // Verify embedded GlobalOptions defaults + if !opts.ExportedOnly { + t.Error("ExportedOnly should be true by default") + } + if opts.IncludeTests { + t.Error("IncludeTests should be false by default") + } + if opts.Format != "md" { t.Errorf("Format should be 'md', got %q", opts.Format) }gnodoc/model/doc_func_test.go (1)
90-159: Good coverage; consider additional edge cases for completeness.The
FullSignaturetests cover the common cases well. For comprehensive coverage, consider adding tests for:
- Named return values (e.g.,
func Foo() (n int, err error))- Variadic parameters (e.g.,
func Printf(format string, args ...interface{}))- Pointer receiver formatting (e.g.,
func (f *Foo) Method())These can be addressed in a follow-up if not critical for the initial implementation.
gnodoc/render/options.go (1)
40-45: Consider nil receiver safety.If
ShouldRenderis called on a nil*RenderOptions, it will panic. Consider adding a nil check or documenting that callers must ensure a non-nil receiver.♻️ Optional: Add nil receiver guard
func (o *RenderOptions) ShouldRender(exported bool) bool { + if o == nil { + return exported // default to exported-only behavior + } if !o.ExportedOnly { return true } return exported }gnodoc/README.md (1)
13-19: Minor markdown formatting inconsistencies flagged by linter.The tables at lines 14, 22, and 31 should be surrounded by blank lines per MD058. Additionally, line 64 uses a tilde code fence while the rest of the document uses backticks (MD048).
📝 Suggested fix for table spacing
### Output + | Flag | Default | Description | | --- | --- | --- | | `--out=DIR` | current directory | Output directory for generated Markdown. | | `--output-file=NAME` | `README.md` | Output filename for the default command. | | `--filename=NAME` | `README.md` | Output filename for `export`. | | `--format=md` | `md` | Output format for `export` (Markdown only). | + ### Parsing and FilteringApply similar spacing around the other tables and use backticks instead of tildes at line 64.
gnodoc/model/doc_package_test.go (1)
137-174: Good test coverage for BuildIndex, consider adding Vars.The test validates that Consts, Funcs, and Types are properly indexed. You might want to add a
Varsgroup to ensure parity withConstshandling, though this may already be covered if they share the same code path.gnodoc/model/doc_type.go (1)
47-67: Consider addingExportedConstructors()for API consistency.
ExportedFields()andExportedMethods()are provided, but there's no correspondingExportedConstructors()method. If the renderer needs to filter constructors by export status (consistent with other filtering patterns), this method would be useful.Optional addition
// ExportedConstructors returns only the exported constructors. func (t DocType) ExportedConstructors() []DocFunc { var result []DocFunc for _, c := range t.Constructors { if c.IsExported() { result = append(result, c) } } return result }gnodoc/model/doc_package.go (1)
61-97: Consider pre-allocating the Index slice for better performance.The
BuildIndexmethod correctly uses a pointer receiver since it mutates the struct. However, when appending top.Indexin a loop, you could estimate capacity upfront to reduce allocations.♻️ Optional: Pre-allocate Index slice
func (p *DocPackage) BuildIndex() { - p.Index = nil + // Estimate capacity to reduce allocations + capacity := 0 + for _, group := range p.Consts { + capacity += len(group.Specs) + } + for _, group := range p.Vars { + capacity += len(group.Specs) + } + capacity += len(p.Funcs) + len(p.Types) + p.Index = make([]DocIndexItem, 0, capacity) // Add constants for _, group := range p.Consts {gnodoc/parser/parser_test.go (1)
134-137: Consider renamingTestMainto avoid shadowing the test framework'sTestMain.The function name
TestMainin the test file content could be confusing sinceTestMainhas special meaning in Go's testing framework. While this is just string content for the parser test, consider using a different name likeTestSomethingfor clarity.gnodoc/parser/parser.go (2)
188-203: Consider adding a timeout to thego listcommand.The
exec.Command("go", "list", ...)call has no timeout, which could cause the CLI to hang indefinitely if Go tooling is misconfigured or the network is slow (for module downloads).♻️ Add context with timeout
+import ( + "context" + "time" +) + // resolveImportPath resolves a Go import path to a directory path. func (p *Parser) resolveImportPath(importPath string) (string, error) { // Use go list to resolve the import path - cmd := exec.Command("go", "list", "-f", "{{.Dir}}", importPath) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "go", "list", "-f", "{{.Dir}}", importPath) output, err := cmd.Output() if err != nil { + if ctx.Err() == context.DeadlineExceeded { + return "", fmt.Errorf("timeout resolving import path %q", importPath) + } return "", fmt.Errorf("cannot resolve import path %q: %w", importPath, err) }
814-859: Consider extracting duplicated return info extraction logic.The AST inspection logic in
collectReturnInfo(lines 831-852) andfallbackReturnInfo(lines 892-913) is nearly identical. This could be extracted into a shared helper function.♻️ Extract shared helper
func (p *Parser) extractReturnInfoFromBody(body *ast.BlockStmt) returnInfo { var info returnInfo ast.Inspect(body, func(n ast.Node) bool { ret, ok := n.(*ast.ReturnStmt) if !ok { return true } if len(ret.Results) == 0 { info.naked = true return true } var exprs []string for _, expr := range ret.Results { if ident, ok := expr.(*ast.Ident); ok { exprs = append(exprs, ident.Name) continue } exprs = append(exprs, p.exprString(expr)) } if len(exprs) > 0 { info.exprs = append(info.exprs, strings.Join(exprs, ", ")) } return true }) return info }Also applies to: 861-918
gnodoc/cmd/runner.go (2)
108-113: Simplify the--all/--exported-onlyflag handling.The current logic is slightly redundant. Since
exportedOnlydefaults totruevia flag definition, theelse ifbranch will always set it totrue. The logic can be simplified.♻️ Simplified flag handling
- // --all overrides --exported-only - if all { - opts.ExportedOnly = false - } else if exportedOnly { - opts.ExportedOnly = true - } + // --all overrides --exported-only + opts.ExportedOnly = !all
279-323: Silent error handling infilepath.Walkmay hide issues.The walk callback returns
nilfor all errors (lines 281 and 291), silently skipping directories that can't be accessed. Consider logging these errors to stderr when they occur, so users are aware of skipped paths.♻️ Log skipped directories
err = filepath.Walk(rootPath, func(path string, info os.FileInfo, err error) error { if err != nil { + fmt.Fprintf(r.stderr, "warning: skipping %s: %v\n", path, err) return nil // Skip errors } if !info.IsDir() { return nil } // Check for Go/Gno files entries, err := os.ReadDir(path) if err != nil { + fmt.Fprintf(r.stderr, "warning: cannot read %s: %v\n", path, err) return nil }gnodoc/render/markdown.go (2)
250-326: Consider extracting shared logic between RenderConstants and RenderVariables.Both methods share nearly identical structure: checking for exported specs, rendering group docs, anchors, code blocks, and source links. A helper accepting the declaration keyword (
const/var) and a value formatter could reduce duplication.This is optional since both implementations are correct.
429-432: Consider extracting the magic number or documenting the baseline count.The check
len(fieldLines) > 2relies on the header and empty line being exactly 2 items. Same pattern appears at lines 463 and 482. A named constant or comment would clarify intent and reduce fragility if the header format changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
gnodoc/.gitignoregnodoc/README.mdgnodoc/cmd/exitcode.gognodoc/cmd/exitcode_test.gognodoc/cmd/options.gognodoc/cmd/options_test.gognodoc/cmd/runner.gognodoc/cmd/runner_test.gognodoc/go.modgnodoc/main.gognodoc/model/doc_extra.gognodoc/model/doc_extra_test.gognodoc/model/doc_field.gognodoc/model/doc_field_test.gognodoc/model/doc_func.gognodoc/model/doc_func_test.gognodoc/model/doc_index.gognodoc/model/doc_index_test.gognodoc/model/doc_node.gognodoc/model/doc_node_test.gognodoc/model/doc_package.gognodoc/model/doc_package_test.gognodoc/model/doc_param.gognodoc/model/doc_param_test.gognodoc/model/doc_type.gognodoc/model/doc_type_test.gognodoc/model/doc_value.gognodoc/model/doc_value_test.gognodoc/model/source_file.gognodoc/model/source_file_test.gognodoc/model/source_pos.gognodoc/model/source_pos_test.gognodoc/parser/options.gognodoc/parser/parser.gognodoc/parser/parser_test.gognodoc/render/anchor.gognodoc/render/anchor_test.gognodoc/render/markdown.gognodoc/render/markdown_test.gognodoc/render/options.gognodoc/render/options_test.go
🧰 Additional context used
🧬 Code graph analysis (20)
gnodoc/cmd/exitcode_test.go (1)
gnodoc/cmd/exitcode.go (4)
ExitSuccess(8-8)ExitError(11-11)ExitPartialError(15-15)ExitCode(4-4)
gnodoc/cmd/runner_test.go (2)
gnodoc/cmd/runner.go (1)
NewRunner(48-53)gnodoc/cmd/exitcode.go (3)
ExitSuccess(8-8)ExitError(11-11)ExitPartialError(15-15)
gnodoc/model/doc_type.go (3)
gnodoc/model/doc_node.go (1)
DocNode(24-34)gnodoc/model/doc_field.go (1)
DocField(9-13)gnodoc/model/doc_func.go (1)
DocFunc(6-17)
gnodoc/model/doc_value.go (1)
gnodoc/model/doc_node.go (1)
DocNode(24-34)
gnodoc/model/doc_extra.go (1)
gnodoc/model/source_pos.go (1)
SourcePos(7-11)
gnodoc/model/doc_index_test.go (2)
gnodoc/model/doc_index.go (2)
DocIndexItem(4-9)NewIndexFromNode(17-24)gnodoc/model/doc_node.go (2)
KindFunc(11-11)DocNode(24-34)
gnodoc/cmd/options_test.go (1)
gnodoc/cmd/options.go (3)
DefaultGlobalOptions(28-35)DefaultExportOptions(52-59)DefaultListOptions(71-76)
gnodoc/model/doc_param_test.go (1)
gnodoc/model/doc_param.go (2)
DocParam(6-9)DocReceiver(21-24)
gnodoc/parser/parser_test.go (2)
gnodoc/parser/parser.go (1)
New(32-47)gnodoc/parser/options.go (1)
DefaultOptions(22-30)
gnodoc/main.go (1)
gnodoc/cmd/runner.go (1)
NewRunner(48-53)
gnodoc/model/doc_index.go (1)
gnodoc/model/doc_node.go (2)
SymbolKind(6-6)DocNode(24-34)
gnodoc/model/source_pos_test.go (1)
gnodoc/model/source_pos.go (1)
SourcePos(7-11)
gnodoc/cmd/runner.go (5)
gnodoc/cmd/exitcode.go (4)
ExitCode(4-4)ExitError(11-11)ExitSuccess(8-8)ExitPartialError(15-15)gnodoc/cmd/options.go (5)
DefaultGlobalOptions(28-35)DefaultExportOptions(52-59)GlobalOptions(4-25)DefaultListOptions(71-76)ListOptions(62-68)gnodoc/parser/options.go (2)
Options(4-19)DefaultOptions(22-30)gnodoc/parser/parser.go (1)
New(32-47)gnodoc/render/markdown.go (1)
NewMarkdownRenderer(19-27)
gnodoc/model/doc_node_test.go (1)
gnodoc/model/doc_node.go (8)
SymbolKind(6-6)KindConst(9-9)KindVar(10-10)KindFunc(11-11)KindType(12-12)KindMethod(13-13)KindField(14-14)DocNode(24-34)
gnodoc/render/markdown.go (8)
gnodoc/render/options.go (2)
RenderOptions(4-25)DefaultOptions(28-36)gnodoc/render/anchor.go (4)
AnchorRegistry(36-40)NewAnchorRegistry(43-49)ToAnchor(12-25)MethodAnchor(29-33)gnodoc/model/doc_package.go (1)
DocPackage(7-22)gnodoc/model/doc_node.go (2)
SymbolKind(6-6)KindMethod(13-13)gnodoc/model/source_pos.go (1)
SourcePos(7-11)gnodoc/model/doc_type.go (1)
TypeKind(4-4)gnodoc/model/doc_extra.go (1)
DocNote(35-39)gnodoc/model/doc_func.go (1)
DocFunc(6-17)
gnodoc/model/source_file_test.go (1)
gnodoc/model/source_file.go (1)
SourceFile(7-10)
gnodoc/model/doc_type_test.go (4)
gnodoc/model/doc_type.go (6)
TypeKind(4-4)TypeKindStruct(7-7)TypeKindInterface(8-8)TypeKindAlias(9-9)TypeKindOther(10-10)DocType(19-25)gnodoc/model/doc_func.go (1)
DocFunc(6-17)gnodoc/model/doc_node.go (1)
DocNode(24-34)gnodoc/model/doc_field.go (1)
DocField(9-13)
gnodoc/model/doc_node.go (2)
gnodoc/model/source_pos.go (1)
SourcePos(7-11)gnodoc/model/doc_extra.go (1)
DocDeprecated(47-50)
gnodoc/model/doc_extra_test.go (2)
gnodoc/model/doc_extra.go (3)
DocExample(6-12)DocNote(35-39)DocDeprecated(47-50)gnodoc/model/source_pos.go (1)
SourcePos(7-11)
gnodoc/render/options_test.go (1)
gnodoc/render/options.go (2)
DefaultOptions(28-36)RenderOptions(4-25)
🪛 markdownlint-cli2 (0.18.1)
gnodoc/README.md
14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
22-22: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
31-31: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
64-64: Code fence style
Expected: backtick; Actual: tilde
(MD048, code-fence-style)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/staker, contract/r/scenario/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/gov/governance/v1, contract/r/gnoswap/gov/governance/v1)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (100)
gnodoc/model/source_file_test.go (1)
16-42: Test summary mentions fallback behavior that doesn't exist.The AI summary states
BaseName()"uses the basename of Path" as fallback, but the implementation simply returnsName. Consider adding a test case for whenNameis empty to document expected behavior, or updateBaseName()to fall back to extracting basename fromPath.gnodoc/parser/options.go (1)
1-30: LGTM!The
Optionsstruct is well-designed with clear documentation for each field. Default values are appropriate for typical documentation generation (exported symbols only, no tests by default).Note: The
IncludeTestscomment correctly mentions*_test.gnofiles, which aligns with my earlier comment onmodel/source_file.goneeding to handle.gnotest files.gnodoc/go.mod (1)
3-3: No action needed. Go version1.25.5is valid and is the latest stable release as of January 2026.Likely an incorrect or invalid review comment.
gnodoc/cmd/exitcode.go (1)
1-30: LGTM!Clean implementation of exit code constants with appropriate documentation. The
String()method correctly handles all defined cases plus anunknownfallback for defensive handling of unexpected values.gnodoc/cmd/exitcode_test.go (1)
1-37: LGTM!Comprehensive test coverage for exit code values and their string representations. Good use of table-driven tests and inclusion of the unknown code edge case.
gnodoc/render/anchor.go (4)
64-71:RegisterNameoverwrites previous mapping on duplicate names.When the same
nameis registered twice,RegisterNameoverwrites the entry inr.nameswith the new unique anchor. This meansGet(name)will return the last registered anchor, not the first. If the intent is to always retrieve the first anchor for a given name, this is a bug.For example:
reg.RegisterName("Foo") // returns "foo", stores names["Foo"] = "foo" reg.RegisterName("Foo") // returns "foo-1", overwrites names["Foo"] = "foo-1" reg.Get("Foo") // returns "foo-1", not "foo"If this is intentional behavior, consider documenting it. If the first anchor should be preserved, add a check:
Suggested fix to preserve first registration
func (r *AnchorRegistry) RegisterName(name string) string { + if existing, ok := r.names[name]; ok { + return existing + } anchor := ToAnchor(name) unique := r.Register(anchor) r.names[name] = unique return unique }
9-33: LGTM!
ToAnchorandMethodAnchorare well-implemented. The anchor conversion correctly handles the documented character classes, and the method anchor properly strips the pointer prefix before generating the combined anchor.
35-62: LGTM!The
AnchorRegistrystruct andRegistermethod correctly implement collision avoidance with numeric suffixes. The count tracking ensures unique anchors across repeated registrations.
73-94: LGTM!
RegisterKeyprovides correct idempotent behavior by returning existing anchors for already-registered keys.GetKeyandGetare straightforward lookups.gnodoc/render/anchor_test.go (4)
1-56: LGTM!Comprehensive table-driven tests for
ToAnchorcovering lowercase conversion, underscore preservation, space-to-dash conversion, special character removal, and digit preservation.
58-84: LGTM!Good test coverage for
Registercollision handling, verifying the suffix increments correctly (foo,foo-1,foo-2) and that different anchors remain unsuffixed.
86-118: Tests align with currentRegisterNameoverwrite behavior.Note that
TestAnchorRegistry_RegisterName(lines 95-99) verifies that registering the same name twice returns"newfoo-1"for the second call. This test would need updating ifRegisterNameis changed to return the existing anchor for duplicate names (as mentioned in the anchor.go review).
120-150: LGTM!Good coverage of
MethodAnchorincluding the pointer receiver case where the*prefix is stripped.gnodoc/model/doc_node_test.go (5)
1-26: LGTM!Comprehensive test coverage for
SymbolKind.String()verifying all six defined kinds.
28-40: LGTM!Good defensive test verifying the zero-value behavior of
DocNodestruct fields.
42-73: LGTM!Tests correctly verify that
IsExported()returns the value of theExportedfield, including the case where the name starts with uppercase butExportedis explicitly false.
75-106: LGTM!Good coverage including the important edge case of whitespace-only documentation being treated as no doc.
108-139: LGTM!Tests verify that
AnchorID()returns the node name for different symbol kinds.gnodoc/main.go (1)
9-12: LGTM!Clean and minimal entry point. The exit code propagation from
Runner.Runtoos.Exitis correctly implemented.gnodoc/model/doc_type_test.go (4)
5-24: LGTM!Good table-driven test structure for
TypeKind.String().
26-44: LGTM!Zero-value test ensures clean struct initialization defaults.
46-164: LGTM!Table-driven tests for
IsStruct,IsInterface,HasMethods, andHasConstructorsare well-structured and cover the relevant cases.
166-206: LGTM!
ExportedFieldsandExportedMethodstests correctly verify both filtering by theExportedflag and preservation of order.gnodoc/render/options_test.go (1)
31-67: LGTM!Good table-driven test covering the key
ShouldRendervisibility combinations.gnodoc/model/doc_param_test.go (2)
5-47: LGTM!Comprehensive tests for
DocParamzero value andString()method covering named, unnamed, and pointer type cases.
49-119: LGTM!Good coverage of
DocReceiverbehavior includingString()formatting for value/pointer/unnamed receivers andIsPointer()detection.gnodoc/model/source_pos.go (1)
5-31: LGTM!Clean implementation of
SourcePosfollowing Go conventions (similar totoken.Position). TheIsValid()andString()methods are well-documented and handle edge cases appropriately.gnodoc/model/doc_field_test.go (4)
5-17: LGTM!Zero-value test correctly validates default field values for DocField.
19-40: LGTM!Properly tests field initialization with DocNode embedding and validates Name, Type, and Tag assignments.
42-68: LGTM!Table-driven test for
HasTag()is idiomatic and covers the essential cases.
70-111: LGTM!Comprehensive test coverage for
TagValue()including edge cases for multiple tags and missing keys.gnodoc/model/source_pos_test.go (3)
5-17: LGTM!Zero-value test correctly validates default field values for SourcePos.
19-60: LGTM!Comprehensive table-driven tests for
IsValid()covering all edge cases including the optional column behavior.
62-93: LGTM!Good coverage for
String()formatting including the special cases of omitted column and invalid position returning "-".gnodoc/cmd/options_test.go (2)
5-20: LGTM!Test correctly validates all default values for GlobalOptions.
36-45: LGTM!Test correctly validates ListOptions defaults.
gnodoc/model/doc_index.go (3)
4-9: LGTM!Clean struct definition with appropriate fields for documentation index items.
11-14: LGTM!Simple and correct anchor link generation.
16-24: LGTM!Clean factory function that correctly maps DocNode fields to DocIndexItem, using
AnchorID()for consistent anchor generation.gnodoc/model/doc_index_test.go (4)
5-20: LGTM!Zero-value test correctly validates all default field values for DocIndexItem.
22-42: LGTM!Test correctly validates field assignments using the
KindFuncconstant.
44-70: LGTM!Table-driven test covers
AnchorLink()including the edge case of an empty anchor.
72-93: LGTM!Test validates that
NewIndexFromNodecorrectly maps DocNode fields to DocIndexItem, including the implicitAnchorID()call for the Anchor field.gnodoc/model/doc_value_test.go (1)
1-148: LGTM!The test file provides solid coverage for
DocValueSpecandDocValueGrouptypes:
- Zero-value behavior is verified for both types
- Field assignments and accessors are tested
- Table-driven tests for
HasExported()cover empty, all-unexported, and mixed-exported scenariosNames()method is verified for correct orderinggnodoc/model/doc_extra_test.go (1)
1-180: LGTM!Comprehensive test coverage for the documentation extras:
- All three types (
DocExample,DocNote,DocDeprecated) have zero-value and populated-value tests- Helper methods (
HasOutput,Suffix,IsBug) are tested with table-driven tests covering relevant casesSourcePos.IsValid()is appropriately tested for populated structsgnodoc/model/doc_param.go (1)
1-38: LGTM!Clean implementation of
DocParamandDocReceiverwith well-documented methods:
String()methods correctly handle both named and unnamed casesIsPointer()usesstrings.HasPrefixappropriately for pointer detection- Documentation comments follow Go conventions and clearly explain formatting
gnodoc/render/options.go (1)
1-36: LGTM!Well-structured configuration type with:
- Clear field documentation explaining each option's purpose
- Sensible defaults in
DefaultOptions()(exported-only, all sections included)- Good example for
SourceLinkBaseusagegnodoc/README.md (1)
1-114: Documentation is comprehensive and well-structured.The README provides clear explanations of module resolution, file discovery, CLI flags, usage examples, and the Markdown output structure. This serves as an excellent reference for users of the gnodoc tool.
gnodoc/model/doc_extra.go (2)
19-31: Suffix() logic is correct and handles edge cases well.The method properly handles:
- Non-Example names (returns original)
- "Example" → ""
- "Example_Foo" → "Foo"
- "ExampleFoo" → "Foo"
33-50: DocNote and DocDeprecated types are well-designed.Clean, minimal structures with appropriate helper methods. The
IsBug()helper aligns with Go's convention for documentation notes.gnodoc/model/doc_field.go (1)
20-49: TagValue implementation is reasonable with best-effort fallback.The primary path using
reflect.StructTag.Get()is correct. The fallback manual parsing handles cases where the tag format might not matchreflect.StructTagexpectations (e.g., tags stored without surrounding backticks).Note that the fallback's space-split approach at line 35 may not handle quoted values containing spaces correctly, but this is acceptable for a documentation tool where perfect tag parsing isn't critical.
gnodoc/model/doc_value.go (1)
17-34: Clean and efficient implementations.
Names()correctly pre-allocates the slice, andHasExported()uses early return for efficiency. Both methods leverage the embeddedDocNodeappropriately.gnodoc/model/doc_package_test.go (1)
1-234: Comprehensive test suite with good coverage.Tests cover zero-value behavior, populated state, doc presence checks, export filtering, index building, and examples/notes presence. Table-driven tests are used appropriately for boolean method checks. The test structure follows Go conventions well.
gnodoc/model/doc_func.go (3)
1-17: Well-structured data model for function documentation.The
DocFuncstruct cleanly separates signature metadata (Params,Results,Receiver) from flow analysis fields (ReturnNames,ReturnExprs,HasNakedReturn). The embedding ofDocNodeprovides consistent base metadata across all symbol types.
19-31: LGTM!The
IsMethod()andReceiverType()methods are simple, correct, and nil-safe.
33-74: Solid signature construction logic.The
FullSignature()method correctly handles:
- Optional receiver prefix
- Parameter list formatting
- Single unnamed result without parentheses (Go convention)
- Multiple/named results with parentheses
The use of
strings.Builderis appropriate for this string assembly pattern.gnodoc/render/markdown_test.go (7)
1-9: Good test file structure.Clean imports and package declaration. The test suite exercises the public API of
MarkdownRendererwithout relying on implementation details.
10-84: Comprehensive coverage for Overview and Index rendering.Tests validate presence of package name, import path, documentation content, and index links. The assertions correctly check for expected Markdown structure.
86-153: Good coverage for Constants and Variables sections.Tests verify section headers and content rendering with appropriate model fixtures.
155-254: Thorough function and type rendering tests.The
TestMarkdownRenderer_FunctionsandTestMarkdownRenderer_Typestests validate signature rendering, documentation inclusion, and method handling.
256-322: Examples and Notes tests are well-structured.Tests validate code block formatting, output sections, and note/deprecation rendering.
324-348: ExportedOnly filtering test is correct.Validates that
ExportedOnlyoption properly filters unexported symbols from the output.
350-450: FullRender and EmptyPackage tests provide good integration coverage.The section ordering validation (Overview → Index → Constants) and empty package handling ensure the renderer behaves correctly in edge cases.
gnodoc/cmd/runner_test.go (7)
11-40: Well-structured test helper.The
createTestPackagehelper properly usest.Helper()and creates isolated test fixtures.
73-98: Good pattern for output directory isolation.This test correctly uses a separate temporary directory for output, ensuring clean isolation.
100-121: Error path tests are appropriate.Tests for invalid paths and missing arguments correctly expect
ExitError.
123-185: Subcommand tests (export, list, help) are well-structured.Tests validate the export subcommand with custom filename, list output, and help behavior.
187-270: ExportedOnly flag test is thorough.Validates that exported symbols appear and unexported symbols are excluded when using
--exported-only.
272-336: Partial error handling test is valuable.Tests the
--ignore-parse-errorsbehavior with a package containing a syntax error, validating that valid files are still documented withExitPartialError.
338-453: --all and --exclude flag tests provide good coverage.These tests validate that
--allincludes unexported symbols and--excludeproperly filters files by pattern.gnodoc/model/doc_node.go (3)
1-20: Clean enum definition with appropriate constants.The
SymbolKindtype and its constants provide clear categorization for documented symbols. TheString()method enables easy debugging and logging.
22-45: Well-designed base struct for documentation metadata.
DocNodeprovides a consistent foundation for all symbol types. TheHasDoc()method correctly trims whitespace before checking for empty documentation.
47-51: Collision handling is already implemented at the renderer level.The renderer doesn't use
DocNode.AnchorID()for generating anchor links. Instead, it callssymbolAnchor(kind, name)(lines 78–86 ingnodoc/render/markdown.go), which creates logical keys that include both theSymbolKindand name:fmt.Sprintf("%s:%s", kind, name). TheAnchorRegistryuses these keys to prevent collisions across different symbol kinds (e.g.,type:Fooandfunc:Fooare distinct entries).DocNode.AnchorID()is either unused or only referenced inNewIndexFromNode(), which itself doesn't appear to be the source of truth for anchor generation in the actual renderer.Likely an incorrect or invalid review comment.
gnodoc/model/doc_type.go (2)
1-16: Clean type kind enumeration.The
TypeKindconstants cover the essential Go type categories. The "other" category provides a sensible fallback for type aliases to non-struct/interface types.
18-45: Well-structured type documentation model.The
DocTypestruct cleanly organizes fields, methods, and constructors. The predicate methods are simple and correct.gnodoc/model/doc_package.go (2)
1-27: LGTM! Well-structured package documentation model.The
DocPackagestruct provides a comprehensive container for package documentation with appropriate fields for all documentation elements. The use of a value receiver forHasDoc()is correct for a read-only method.
29-59: LGTM! Clean filtering and predicate methods.The
ExportedFuncs,ExportedTypes,HasExamples, andHasNotesmethods are implemented correctly with value receivers since they don't mutate state.gnodoc/cmd/options.go (2)
1-35: LGTM! Clean option structures with sensible defaults.The
GlobalOptionsstruct and its constructor provide clear, well-documented configuration options with appropriate defaults.
61-76: LGTM! Simple list options.The
ListOptionsstruct is appropriately minimal for its purpose.gnodoc/parser/parser_test.go (3)
10-113: LGTM! Comprehensive basic parsing test.The test thoroughly validates package parsing, including constants, variables, types with methods and constructors. Good use of temp directories for isolation.
737-830: LGTM! Good module root and gnomod.toml detection tests.These tests properly validate relative path handling and automatic module root detection from
gnomod.toml. The test setup is clean and assertions are appropriate.
678-734: Syntax is intentionally supported by the parser for multi-name value assignments.The test content
const A, B = 1andvar X, Y = 2is valid for this Gno parser. The parser has explicit logic (if len(vs.Values) == 1) that assigns a single value to multiple names. The test name "TestParser_ValueSpec_MultipleNamesSingleValue" confirms this is a deliberate feature, and the parser correctly assigns the value "1" to both A and B, and "2" to both X and Y. No fix is required.Likely an incorrect or invalid review comment.
gnodoc/parser/parser.go (3)
1-47: LGTM! Clean parser initialization.The
Parserstruct andNewconstructor are well-structured with appropriate nil handling for options.
920-951: LGTM! Clean example conversion with proper normalization.The
convertExamplefunction handles bothCodeandPlaynodes appropriately, with formatting and normalization.
224-258: ThereadGnoModModulefunction adequately handles the actual gnomod.toml format used in practice. All files in the codebase use the simplemodule = "value"format at the root level, with no multi-line strings, escaped quotes, or other complex TOML features in the module field itself. The function's early return upon finding the "module" key means it never encounters the bracketed sections ([replace],[addpkg]) that do exist in the TOML spec. No refactoring is required.gnodoc/cmd/runner.go (3)
1-53: LGTM! Clean runner structure with dependency injection.The
Runnerstruct with configurablestdout/stderrwriters enables testability and flexibility.
55-80: LGTM! Clear command dispatch logic.The
Runmethod handles help flags and subcommand routing cleanly.
212-259: LGTM! Well-structured documentation generation flow.The
generateDocmethod correctly:
- Creates parser with appropriate options
- Handles parse errors
- Ensures output directory exists
- Writes the output file
- Reports partial errors via
ExitPartialErrorgnodoc/render/markdown.go (12)
1-9: LGTM!Clean imports with minimal dependencies. Standard library usage is appropriate for string manipulation and formatting.
11-27: LGTM!Good defensive handling of nil options with fallback to defaults. The struct design cleanly separates configuration, state (anchors), and output accumulation (sections).
29-72: LGTM!The render orchestration is clean with proper section ordering and conditional inclusion. Resetting anchors on each call ensures the renderer is reusable across multiple packages.
74-93: LGTM!Helper methods are well-factored. The
sourceLinkcorrectly validates both the base URL and position validity before generating links.
95-114: LGTM!Clean overview rendering with proper handling of optional import path and documentation.
116-166: LGTM!Index generation properly filters by export status and generates consistent anchor links. The structure cleanly collects all symbol types.
168-248: LGTM!The two-pass approach efficiently checks for exported content before rendering. Group documentation and source links are properly handled.
328-377: LGTM!Clean function rendering with proper signature display and returns documentation. The horizontal rule separators improve readability between function entries.
502-547: LGTM!Examples are rendered cleanly with proper handling of optional documentation and output sections. Using
ex.Suffix()for display names is a nice touch.
549-595: LGTM!Notes are properly grouped and sorted for deterministic output. The blockquote formatting with bold labels provides clear visual distinction.
597-638: LGTM!Return rendering properly handles all cases: named returns, naked returns, and expressions. The deduplication with nil-at-end ordering matches the PR objectives for stable presentation.
398-402: The code is functioning correctly. The parser always populatesSignaturealongsideTypeKindin all type processing paths (struct, interface, and alias cases at lines 586, 594, and 597 ingnodoc/parser/parser.go). The fallback in markdown.go would produce identical output:"type Name struct"for structs,"type Name interface"for interfaces. These documentation-style signatures intentionally omit details like braces and are consistent with Go's documentation conventions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // ExportOptions contains options for the export command. | ||
| type ExportOptions struct { | ||
| GlobalOptions | ||
|
|
||
| // Format is the output format (md). | ||
| Format string | ||
|
|
||
| // OutputDir is the output directory. | ||
| OutputDir string | ||
|
|
||
| // Filename is the output filename. | ||
| Filename string | ||
| } | ||
|
|
||
| // DefaultExportOptions returns the default export options. | ||
| func DefaultExportOptions() *ExportOptions { | ||
| return &ExportOptions{ | ||
| GlobalOptions: *DefaultGlobalOptions(), | ||
| Format: "md", | ||
| OutputDir: ".", | ||
| Filename: "README.md", | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential confusion: ExportOptions has both OutputFile (embedded) and Filename.
ExportOptions embeds GlobalOptions which has OutputFile, but also declares its own Filename field. Both are set to "README.md" in DefaultExportOptions(). This creates ambiguity about which field controls the output filename.
Looking at runner.go line 177, there's explicit synchronization: opts.OutputFile = opts.Filename. Consider either removing one field or documenting the relationship clearly.
💡 Suggested fix: Remove duplicate field
// ExportOptions contains options for the export command.
type ExportOptions struct {
GlobalOptions
// Format is the output format (md).
Format string
// OutputDir is the output directory.
OutputDir string
-
- // Filename is the output filename.
- Filename string
}
// DefaultExportOptions returns the default export options.
func DefaultExportOptions() *ExportOptions {
return &ExportOptions{
GlobalOptions: *DefaultGlobalOptions(),
Format: "md",
OutputDir: ".",
- Filename: "README.md",
}
}| func TestRunner_Run_BasicCommand(t *testing.T) { | ||
| tmpDir := createTestPackage(t) | ||
| defer os.RemoveAll(tmpDir) | ||
|
|
||
| var stdout, stderr bytes.Buffer | ||
| r := NewRunner(&stdout, &stderr) | ||
|
|
||
| // Run with path argument | ||
| code := r.Run([]string{tmpDir}) | ||
|
|
||
| if code != ExitSuccess { | ||
| t.Errorf("expected ExitSuccess, got %v: %s", code, stderr.String()) | ||
| } | ||
|
|
||
| // Check output file was created | ||
| outputPath := filepath.Join(".", "README.md") | ||
| content, err := os.ReadFile(outputPath) | ||
| if err != nil { | ||
| t.Fatalf("failed to read output file: %v", err) | ||
| } | ||
| defer os.Remove(outputPath) | ||
|
|
||
| // Verify content contains expected sections | ||
| if !strings.Contains(string(content), "# testpkg") { | ||
| t.Error("expected package name in output") | ||
| } | ||
| if !strings.Contains(string(content), "Foo") { | ||
| t.Error("expected Foo type in output") | ||
| } | ||
| } |
There was a problem hiding this comment.
Test cleanup issue: README.md written to working directory may persist on test failure.
The test writes README.md to the current working directory (line 57-58), and the cleanup (os.Remove) is deferred after the read operations. If the test fails between lines 58-61, the file remains and could pollute subsequent test runs or the repository.
Consider using a dedicated output directory (like other tests do) or ensuring cleanup happens regardless of assertion failures.
Proposed fix
func TestRunner_Run_BasicCommand(t *testing.T) {
tmpDir := createTestPackage(t)
defer os.RemoveAll(tmpDir)
+ outDir, err := os.MkdirTemp("", "gnodoc-basic-out-*")
+ if err != nil {
+ t.Fatalf("failed to create output dir: %v", err)
+ }
+ defer os.RemoveAll(outDir)
+
var stdout, stderr bytes.Buffer
r := NewRunner(&stdout, &stderr)
// Run with path argument
- code := r.Run([]string{tmpDir})
+ code := r.Run([]string{"--out=" + outDir, tmpDir})
if code != ExitSuccess {
t.Errorf("expected ExitSuccess, got %v: %s", code, stderr.String())
}
// Check output file was created
- outputPath := filepath.Join(".", "README.md")
+ outputPath := filepath.Join(outDir, "README.md")
content, err := os.ReadFile(outputPath)
if err != nil {
t.Fatalf("failed to read output file: %v", err)
}
- defer os.Remove(outputPath)
// Verify content contains expected sections
if !strings.Contains(string(content), "# testpkg") {
t.Error("expected package name in output")
}
if !strings.Contains(string(content), "Foo") {
t.Error("expected Foo type in output")
}
}| // IsTestFile reports whether this is a test file. | ||
| func (f SourceFile) IsTestFile() bool { | ||
| return strings.HasSuffix(f.Name, "_test.go") |
There was a problem hiding this comment.
Missing .gno test file detection.
The PR adds support for .gno files, and the parser Options.IncludeTests comment mentions *_test.gno files. However, IsTestFile() only checks for _test.go suffix, missing .gno test files.
Proposed fix to handle .gno test files
// IsTestFile reports whether this is a test file.
func (f SourceFile) IsTestFile() bool {
- return strings.HasSuffix(f.Name, "_test.go")
+ return strings.HasSuffix(f.Name, "_test.go") || strings.HasSuffix(f.Name, "_test.gno")
}📝 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.
| // IsTestFile reports whether this is a test file. | |
| func (f SourceFile) IsTestFile() bool { | |
| return strings.HasSuffix(f.Name, "_test.go") | |
| // IsTestFile reports whether this is a test file. | |
| func (f SourceFile) IsTestFile() bool { | |
| return strings.HasSuffix(f.Name, "_test.go") || strings.HasSuffix(f.Name, "_test.gno") | |
| } |
| case *ast.ArrayType: | ||
| if t.Len == nil { | ||
| return "[]" + p.typeString(t.Elt) | ||
| } | ||
| return "[...]" + p.typeString(t.Elt) |
There was a problem hiding this comment.
Array type with explicit length renders incorrectly as [...].
When an array has an explicit length (e.g., [5]int), the code renders it as [...]int instead of [5]int. The length expression is lost.
🐛 Fix array length rendering
case *ast.ArrayType:
if t.Len == nil {
return "[]" + p.typeString(t.Elt)
}
- return "[...]" + p.typeString(t.Elt)
+ return "[" + p.exprString(t.Len) + "]" + p.typeString(t.Elt)📝 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.
| case *ast.ArrayType: | |
| if t.Len == nil { | |
| return "[]" + p.typeString(t.Elt) | |
| } | |
| return "[...]" + p.typeString(t.Elt) | |
| case *ast.ArrayType: | |
| if t.Len == nil { | |
| return "[]" + p.typeString(t.Elt) | |
| } | |
| return "[" + p.exprString(t.Len) + "]" + p.typeString(t.Elt) |
| func (p *Parser) fallbackReturnInfo(f *doc.Func, receiverType string) (returnInfo, bool) { | ||
| if f.Decl == nil { | ||
| return returnInfo{}, false | ||
| } | ||
| pos := p.fset.Position(f.Decl.Pos()) | ||
| if pos.Filename == "" { | ||
| return returnInfo{}, false | ||
| } | ||
|
|
||
| file, err := parser.ParseFile(token.NewFileSet(), pos.Filename, nil, parser.ParseComments) | ||
| if err != nil { | ||
| return returnInfo{}, false | ||
| } | ||
|
|
||
| for _, decl := range file.Decls { | ||
| fn, ok := decl.(*ast.FuncDecl) | ||
| if !ok || fn.Name == nil || fn.Body == nil { | ||
| continue | ||
| } | ||
| if fn.Name.Name != f.Name { | ||
| continue | ||
| } | ||
| recvType := "" | ||
| if fn.Recv != nil && len(fn.Recv.List) > 0 { | ||
| recvType = p.typeString(fn.Recv.List[0].Type) | ||
| } | ||
| if recvType != receiverType { | ||
| continue | ||
| } | ||
|
|
||
| info := returnInfo{} | ||
| ast.Inspect(fn.Body, func(n ast.Node) bool { | ||
| ret, ok := n.(*ast.ReturnStmt) | ||
| if !ok { | ||
| return true | ||
| } | ||
| if len(ret.Results) == 0 { | ||
| info.naked = true | ||
| return true | ||
| } | ||
| var exprs []string | ||
| for _, expr := range ret.Results { | ||
| if ident, ok := expr.(*ast.Ident); ok { | ||
| exprs = append(exprs, ident.Name) | ||
| continue | ||
| } | ||
| exprs = append(exprs, p.exprString(expr)) | ||
| } | ||
| if len(exprs) > 0 { | ||
| info.exprs = append(info.exprs, strings.Join(exprs, ", ")) | ||
| } | ||
| return true | ||
| }) | ||
| return info, true | ||
| } | ||
|
|
||
| return returnInfo{}, false | ||
| } |
There was a problem hiding this comment.
fallbackReturnInfo creates a new FileSet, causing position mismatches.
Line 870 creates a new token.NewFileSet() instead of reusing p.fset. This means positions from the fallback parsing won't match the main parser's positions, and p.exprString calls within this function use the wrong fset context.
🐛 Reuse the parser's FileSet
func (p *Parser) fallbackReturnInfo(f *doc.Func, receiverType string) (returnInfo, bool) {
if f.Decl == nil {
return returnInfo{}, false
}
pos := p.fset.Position(f.Decl.Pos())
if pos.Filename == "" {
return returnInfo{}, false
}
- file, err := parser.ParseFile(token.NewFileSet(), pos.Filename, nil, parser.ParseComments)
+ file, err := parser.ParseFile(p.fset, pos.Filename, nil, parser.ParseComments)
if err != nil {
return returnInfo{}, false
}📝 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.
| func (p *Parser) fallbackReturnInfo(f *doc.Func, receiverType string) (returnInfo, bool) { | |
| if f.Decl == nil { | |
| return returnInfo{}, false | |
| } | |
| pos := p.fset.Position(f.Decl.Pos()) | |
| if pos.Filename == "" { | |
| return returnInfo{}, false | |
| } | |
| file, err := parser.ParseFile(token.NewFileSet(), pos.Filename, nil, parser.ParseComments) | |
| if err != nil { | |
| return returnInfo{}, false | |
| } | |
| for _, decl := range file.Decls { | |
| fn, ok := decl.(*ast.FuncDecl) | |
| if !ok || fn.Name == nil || fn.Body == nil { | |
| continue | |
| } | |
| if fn.Name.Name != f.Name { | |
| continue | |
| } | |
| recvType := "" | |
| if fn.Recv != nil && len(fn.Recv.List) > 0 { | |
| recvType = p.typeString(fn.Recv.List[0].Type) | |
| } | |
| if recvType != receiverType { | |
| continue | |
| } | |
| info := returnInfo{} | |
| ast.Inspect(fn.Body, func(n ast.Node) bool { | |
| ret, ok := n.(*ast.ReturnStmt) | |
| if !ok { | |
| return true | |
| } | |
| if len(ret.Results) == 0 { | |
| info.naked = true | |
| return true | |
| } | |
| var exprs []string | |
| for _, expr := range ret.Results { | |
| if ident, ok := expr.(*ast.Ident); ok { | |
| exprs = append(exprs, ident.Name) | |
| continue | |
| } | |
| exprs = append(exprs, p.exprString(expr)) | |
| } | |
| if len(exprs) > 0 { | |
| info.exprs = append(info.exprs, strings.Join(exprs, ", ")) | |
| } | |
| return true | |
| }) | |
| return info, true | |
| } | |
| return returnInfo{}, false | |
| } | |
| func (p *Parser) fallbackReturnInfo(f *doc.Func, receiverType string) (returnInfo, bool) { | |
| if f.Decl == nil { | |
| return returnInfo{}, false | |
| } | |
| pos := p.fset.Position(f.Decl.Pos()) | |
| if pos.Filename == "" { | |
| return returnInfo{}, false | |
| } | |
| file, err := parser.ParseFile(p.fset, pos.Filename, nil, parser.ParseComments) | |
| if err != nil { | |
| return returnInfo{}, false | |
| } | |
| for _, decl := range file.Decls { | |
| fn, ok := decl.(*ast.FuncDecl) | |
| if !ok || fn.Name == nil || fn.Body == nil { | |
| continue | |
| } | |
| if fn.Name.Name != f.Name { | |
| continue | |
| } | |
| recvType := "" | |
| if fn.Recv != nil && len(fn.Recv.List) > 0 { | |
| recvType = p.typeString(fn.Recv.List[0].Type) | |
| } | |
| if recvType != receiverType { | |
| continue | |
| } | |
| info := returnInfo{} | |
| ast.Inspect(fn.Body, func(n ast.Node) bool { | |
| ret, ok := n.(*ast.ReturnStmt) | |
| if !ok { | |
| return true | |
| } | |
| if len(ret.Results) == 0 { | |
| info.naked = true | |
| return true | |
| } | |
| var exprs []string | |
| for _, expr := range ret.Results { | |
| if ident, ok := expr.(*ast.Ident); ok { | |
| exprs = append(exprs, ident.Name) | |
| continue | |
| } | |
| exprs = append(exprs, p.exprString(expr)) | |
| } | |
| if len(exprs) > 0 { | |
| info.exprs = append(info.exprs, strings.Join(exprs, ", ")) | |
| } | |
| return true | |
| }) | |
| return info, true | |
| } | |
| return returnInfo{}, false | |
| } |
| func TestRenderOptions_Default(t *testing.T) { | ||
| opts := DefaultOptions() | ||
|
|
||
| if !opts.ExportedOnly { | ||
| t.Error("expected ExportedOnly to be true by default") | ||
| } | ||
| if !opts.IncludeIndex { | ||
| t.Error("expected IncludeIndex to be true by default") | ||
| } | ||
| if !opts.IncludeExamples { | ||
| t.Error("expected IncludeExamples to be true by default") | ||
| } | ||
| if opts.SourceLinkBase != "" { | ||
| t.Error("expected empty SourceLinkBase by default") | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test for IncludeNotes default.
Based on the DefaultOptions() implementation in gnodoc/render/options.go, IncludeNotes is set to true by default, but this isn't verified here.
📝 Suggested addition
if opts.SourceLinkBase != "" {
t.Error("expected empty SourceLinkBase by default")
}
+ if !opts.IncludeNotes {
+ t.Error("expected IncludeNotes to be true by default")
+ }
}📝 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.
| func TestRenderOptions_Default(t *testing.T) { | |
| opts := DefaultOptions() | |
| if !opts.ExportedOnly { | |
| t.Error("expected ExportedOnly to be true by default") | |
| } | |
| if !opts.IncludeIndex { | |
| t.Error("expected IncludeIndex to be true by default") | |
| } | |
| if !opts.IncludeExamples { | |
| t.Error("expected IncludeExamples to be true by default") | |
| } | |
| if opts.SourceLinkBase != "" { | |
| t.Error("expected empty SourceLinkBase by default") | |
| } | |
| } | |
| func TestRenderOptions_Default(t *testing.T) { | |
| opts := DefaultOptions() | |
| if !opts.ExportedOnly { | |
| t.Error("expected ExportedOnly to be true by default") | |
| } | |
| if !opts.IncludeIndex { | |
| t.Error("expected IncludeIndex to be true by default") | |
| } | |
| if !opts.IncludeExamples { | |
| t.Error("expected IncludeExamples to be true by default") | |
| } | |
| if opts.SourceLinkBase != "" { | |
| t.Error("expected empty SourceLinkBase by default") | |
| } | |
| if !opts.IncludeNotes { | |
| t.Error("expected IncludeNotes to be true by default") | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
gnodoc/parser/parser.go (2)
721-725: Array type with explicit length renders incorrectly as[...].When an array has an explicit length (e.g.,
[5]int), the code renders it as[...]intinstead of[5]int. The length expression is lost.🐛 Fix array length rendering
case *ast.ArrayType: if t.Len == nil { return "[]" + p.typeString(t.Elt) } - return "[...]" + p.typeString(t.Elt) + return "[" + p.exprString(t.Len) + "]" + p.typeString(t.Elt)
857-870:fallbackReturnInfocreates a new FileSet, causing position mismatches.Line 866 creates a new
token.NewFileSet()instead of reusingp.fset. This means positions from the fallback parsing won't match the main parser's positions, andp.exprStringcalls within this function use the wrongfsetcontext.🐛 Reuse the parser's FileSet
func (p *Parser) fallbackReturnInfo(f *doc.Func, receiverType string) (returnInfo, bool) { if f.Decl == nil { return returnInfo{}, false } pos := p.fset.Position(f.Decl.Pos()) if pos.Filename == "" { return returnInfo{}, false } - file, err := parser.ParseFile(token.NewFileSet(), pos.Filename, nil, parser.ParseComments) + file, err := parser.ParseFile(p.fset, pos.Filename, nil, parser.ParseComments) if err != nil { return returnInfo{}, false }
🧹 Nitpick comments (5)
gnodoc/model/doc_field.go (1)
33-46: Minor edge case: tag values containing spaces.The manual parsing fallback splits on space, which could incorrectly break up tag values that contain spaces (e.g.,
description:"hello world"). While rare in practice, consider using a more robust parsing approach if such tags are expected.For standard struct tags (e.g.,
json:"id,omitempty" xml:"id"), the primaryreflect.StructTag.Get()path handles them correctly, so this is only a concern if non-standard tag formats are anticipated.gnodoc/parser/parser.go (4)
111-125: Non-deterministic package selection when multiple packages exist.Map iteration order in Go is undefined. If a directory contains multiple non-test packages, the selected
mainPkgwill be arbitrary and may vary between runs. Consider sorting package names or using a deterministic selection strategy.♻️ Suggested fix for deterministic selection
// Get the main package (not test package) var mainPkg *ast.Package + var mainPkgName string for name, pkg := range pkgs { if !strings.HasSuffix(name, "_test") { - mainPkg = pkg - break + if mainPkg == nil || name < mainPkgName { + mainPkg = pkg + mainPkgName = name + } } } if mainPkg == nil { // If only test packages, use first one + var testPkgName string for _, pkg := range pkgs { - mainPkg = pkg - break + for name := range pkgs { + if mainPkg == nil || name < testPkgName { + mainPkg = pkg + testPkgName = name + } + } } }
189-204: External command execution without timeout.
exec.Commandwithout a context or timeout could hang indefinitely ifgo liststalls (e.g., network issues resolving remote modules). For a CLI tool this is lower risk, but consider adding a timeout for robustness.♻️ Suggested fix using context with timeout
+import ( + "context" + "time" +) func (p *Parser) resolveImportPath(importPath string) (string, error) { - cmd := exec.Command("go", "list", "-f", "{{.Dir}}", importPath) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "go", "list", "-f", "{{.Dir}}", importPath) output, err := cmd.Output() if err != nil { return "", fmt.Errorf("cannot resolve import path %q: %w", importPath, err) }
225-259: Manual TOML parsing is fragile.This hand-rolled parser doesn't handle TOML edge cases like escaped quotes (
module = "foo\"bar"), multiline strings, or inline comments. Consider using a proper TOML library likegithub.qkg1.top/BurntSushi/tomlorgithub.qkg1.top/pelletier/go-tomlfor robustness.
1013-1081: Complex indentation normalization logic is hard to follow.The two-pass approach with
minIndentandminIndentNonZerohandles edge cases but is difficult to reason about. Consider adding a comment explaining the intent, or simplifying if the edge cases aren't critical.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gnodoc/model/doc_field.gognodoc/parser/parser.go
🧰 Additional context used
🧬 Code graph analysis (1)
gnodoc/model/doc_field.go (1)
gnodoc/model/doc_node.go (1)
DocNode(24-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/position, contract/r/gnoswap/position)
- GitHub Check: run-integration-test
🔇 Additional comments (2)
gnodoc/model/doc_field.go (2)
8-13: LGTM!Clean struct definition with appropriate embedding of
DocNode. The additionalTypeandTagfields are well-suited for representing struct/interface field metadata.
15-18: LGTM!Simple and correct implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // isExcluded checks if a filename matches any exclude pattern. | ||
| func (p *Parser) isExcluded(name string) bool { | ||
| for _, pattern := range p.opts.Exclude { | ||
| if matched, _ := filepath.Match(pattern, name); matched { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Silent error suppression in pattern matching.
filepath.Match returns an error for malformed patterns (e.g., unclosed brackets). Ignoring this error means users won't know if their exclude pattern is invalid.
🐛 Suggested fix to log or collect pattern errors
func (p *Parser) isExcluded(name string) bool {
for _, pattern := range p.opts.Exclude {
- if matched, _ := filepath.Match(pattern, name); matched {
+ matched, err := filepath.Match(pattern, name)
+ if err != nil {
+ // Invalid pattern - could log warning or collect errors
+ continue
+ }
+ if matched {
return true
}
}
return false
}📝 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.
| // isExcluded checks if a filename matches any exclude pattern. | |
| func (p *Parser) isExcluded(name string) bool { | |
| for _, pattern := range p.opts.Exclude { | |
| if matched, _ := filepath.Match(pattern, name); matched { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| // isExcluded checks if a filename matches any exclude pattern. | |
| func (p *Parser) isExcluded(name string) bool { | |
| for _, pattern := range p.opts.Exclude { | |
| matched, err := filepath.Match(pattern, name) | |
| if err != nil { | |
| // Invalid pattern - could log warning or collect errors | |
| continue | |
| } | |
| if matched { | |
| return true | |
| } | |
| } | |
| return false | |
| } |
| // Field name (may be anonymous) | ||
| if len(field.Names) > 0 { | ||
| f.Name = field.Names[0].Name | ||
| f.Exported = isExported(f.Name) | ||
| f.Pos = p.convertPos(field.Names[0].Pos()) | ||
| } else { | ||
| // Anonymous field - use type name | ||
| f.Name = f.Type | ||
| f.Exported = isExported(f.Name) | ||
| } |
There was a problem hiding this comment.
Multi-name struct fields lose names beyond the first.
For struct fields like a, b, c int, only the first name (a) is captured. The other names (b, c) are silently dropped. This may be intentional for documentation purposes, but could lead to incomplete docs.
🐛 Suggested fix to capture all field names
// Field name (may be anonymous)
- if len(field.Names) > 0 {
- f.Name = field.Names[0].Name
- f.Exported = isExported(f.Name)
- f.Pos = p.convertPos(field.Names[0].Pos())
- } else {
+ if len(field.Names) == 0 {
// Anonymous field - use type name
f.Name = f.Type
f.Exported = isExported(f.Name)
+ } else if len(field.Names) == 1 {
+ f.Name = field.Names[0].Name
+ f.Exported = isExported(f.Name)
+ f.Pos = p.convertPos(field.Names[0].Pos())
+ } else {
+ // Multiple names - join them
+ names := make([]string, len(field.Names))
+ for i, n := range field.Names {
+ names[i] = n.Name
+ }
+ f.Name = strings.Join(names, ", ")
+ f.Exported = isExported(field.Names[0].Name)
+ f.Pos = p.convertPos(field.Names[0].Pos())
}Alternatively, return multiple DocField entries for multi-name fields if the model supports it.
| case *ast.ChanType: | ||
| return "chan " + p.typeString(t.Value) |
There was a problem hiding this comment.
Channel direction is not represented in type string.
chan int, chan<- int, and <-chan int all render as "chan int". The send-only and receive-only directions are lost.
🐛 Suggested fix to include channel direction
case *ast.ChanType:
- return "chan " + p.typeString(t.Value)
+ switch t.Dir {
+ case ast.SEND:
+ return "chan<- " + p.typeString(t.Value)
+ case ast.RECV:
+ return "<-chan " + p.typeString(t.Value)
+ default:
+ return "chan " + p.typeString(t.Value)
+ }📝 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.
| case *ast.ChanType: | |
| return "chan " + p.typeString(t.Value) | |
| case *ast.ChanType: | |
| switch t.Dir { | |
| case ast.SEND: | |
| return "chan<- " + p.typeString(t.Value) | |
| case ast.RECV: | |
| return "<-chan " + p.typeString(t.Value) | |
| default: | |
| return "chan " + p.typeString(t.Value) | |
| } |
| // collectExamplesFromFiles extracts examples from test files listed for the package. | ||
| func (p *Parser) collectExamplesFromFiles(dir string, files []string) []*doc.Example { | ||
| var testFiles []*ast.File | ||
| for _, name := range files { | ||
| if !strings.HasSuffix(name, "_test.go") { | ||
| continue | ||
| } | ||
| path := filepath.Join(dir, name) | ||
| file, err := parser.ParseFile(p.fset, path, nil, parser.ParseComments) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| testFiles = append(testFiles, file) | ||
| } | ||
|
|
||
| if len(testFiles) == 0 { | ||
| return nil | ||
| } | ||
| return doc.Examples(testFiles...) | ||
| } |
There was a problem hiding this comment.
Example collection ignores *_test.gno files.
Line 781 only checks for _test.go suffix, but isTestFile() (line 952) also recognizes _test.gno. This inconsistency means examples in .gno test files won't be collected.
🐛 Suggested fix to also collect from .gno test files
func (p *Parser) collectExamplesFromFiles(dir string, files []string) []*doc.Example {
var testFiles []*ast.File
for _, name := range files {
- if !strings.HasSuffix(name, "_test.go") {
+ if !isTestFile(name) {
continue
}📝 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.
| // collectExamplesFromFiles extracts examples from test files listed for the package. | |
| func (p *Parser) collectExamplesFromFiles(dir string, files []string) []*doc.Example { | |
| var testFiles []*ast.File | |
| for _, name := range files { | |
| if !strings.HasSuffix(name, "_test.go") { | |
| continue | |
| } | |
| path := filepath.Join(dir, name) | |
| file, err := parser.ParseFile(p.fset, path, nil, parser.ParseComments) | |
| if err != nil { | |
| continue | |
| } | |
| testFiles = append(testFiles, file) | |
| } | |
| if len(testFiles) == 0 { | |
| return nil | |
| } | |
| return doc.Examples(testFiles...) | |
| } | |
| // collectExamplesFromFiles extracts examples from test files listed for the package. | |
| func (p *Parser) collectExamplesFromFiles(dir string, files []string) []*doc.Example { | |
| var testFiles []*ast.File | |
| for _, name := range files { | |
| if !isTestFile(name) { | |
| continue | |
| } | |
| path := filepath.Join(dir, name) | |
| file, err := parser.ParseFile(p.fset, path, nil, parser.ParseComments) | |
| if err != nil { | |
| continue | |
| } | |
| testFiles = append(testFiles, file) | |
| } | |
| if len(testFiles) == 0 { | |
| return nil | |
| } | |
| return doc.Examples(testFiles...) | |
| } |
| for i := 0; i < len(lines); { | ||
| line := lines[i] | ||
| trimmed := strings.TrimSpace(line) | ||
| if after, ok := strings.CutPrefix(trimmed, "Deprecated:"); ok { | ||
| body := strings.TrimSpace(after) | ||
| j := i + 1 | ||
| for j < len(lines) { | ||
| next := strings.TrimSpace(lines[j]) | ||
| if next == "" { | ||
| break | ||
| } | ||
| if body != "" { | ||
| body += " " | ||
| } | ||
| body += next | ||
| j++ | ||
| } | ||
| deprecated = append(deprecated, model.DocDeprecated{Body: body, Pos: pos}) | ||
| i = j | ||
| continue | ||
| } |
There was a problem hiding this comment.
Deprecation message parsing may consume unrelated documentation.
The loop at lines 1102-1111 continues appending lines until it hits an empty line. If there's no blank line after the deprecation notice, subsequent documentation gets incorrectly included in the deprecation body.
For example:
// Deprecated: Use NewFoo instead.
// Foo creates a new instance.
func Foo() {}Would result in deprecation body: "Use NewFoo instead. Foo creates a new instance."
🐛 Suggested fix to be more conservative
Consider stopping at lines that look like new documentation sentences (start with uppercase after period) or limiting to a single line unless the next line starts with whitespace (continuation indent).
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
gnodoc/parser/parser.go (4)
627-631: Preserve all names in multi-name struct fields.Fields like
a, b intcurrently keep onlya, droppingbfrom docs. Consider joining names or emitting multipleDocFieldentries. (Duplicate of prior review.)🛠️ Proposed fix (join names)
- if len(field.Names) > 0 { - f.Name = field.Names[0].Name - f.Exported = isExported(f.Name) - f.Pos = p.convertPos(field.Names[0].Pos()) - } else { + if len(field.Names) == 0 { // Anonymous field - use type name f.Name = f.Type f.Exported = isExported(f.Name) + } else if len(field.Names) == 1 { + f.Name = field.Names[0].Name + f.Exported = isExported(f.Name) + f.Pos = p.convertPos(field.Names[0].Pos()) + } else { + names := make([]string, len(field.Names)) + for i, n := range field.Names { + names[i] = n.Name + } + f.Name = strings.Join(names, ", ") + f.Exported = isExported(field.Names[0].Name) + f.Pos = p.convertPos(field.Names[0].Pos()) }
721-725: Render explicit array lengths instead of[...].
[5]intcurrently renders as[...]int, losing the length expression. (Duplicate of prior review.)🛠️ Proposed fix
case *ast.ArrayType: if t.Len == nil { return "[]" + p.typeString(t.Elt) } - return "[...]" + p.typeString(t.Elt) + return "[" + p.exprString(t.Len) + "]" + p.typeString(t.Elt)
728-729: Include channel direction in type strings.Send-only and receive-only channels are currently rendered the same as bidirectional. (Duplicate of prior review.)
🛠️ Proposed fix
case *ast.ChanType: - return "chan " + p.typeString(t.Value) + switch t.Dir { + case ast.SEND: + return "chan<- " + p.typeString(t.Value) + case ast.RECV: + return "<-chan " + p.typeString(t.Value) + default: + return "chan " + p.typeString(t.Value) + }
825-827: Collect examples from_test.gnofiles too.
isTestFilealready recognizes_test.gno, but this loop only includes_test.go, so Gno tests are skipped. (Duplicate of prior review.)🛠️ Proposed fix
for _, name := range files { - if !strings.HasSuffix(name, "_test.go") { + if !isTestFile(name) { continue }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gnodoc/parser/parser.gognodoc/render/markdown.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (1)
gnodoc/parser/parser.go (1)
289-296: RespectIgnoreParseErrorswhen parsing individual files.Line 292 skips parse errors unconditionally even when
IgnoreParseErrorsis false, which can silently drop files and yield partial docs. The error handling should check the flag before deciding to continue.Proposed fix
for _, filename := range files { path := filepath.Join(dir, filename) f, err := parser.ParseFile(p.fset, path, nil, parser.ParseComments) if err != nil { + if !p.opts.IgnoreParseErrors { + return nil, fmt.Errorf("parse error in %s: %w", path, err) + } // Mark that we had parse errors, then skip the failed file p.hadParseErrors = true continue }
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| for _, spec := range group.Specs { | ||
| if !r.opts.ShouldRender(spec.Exported) { | ||
| continue | ||
| } | ||
| if spec.Value != "" { | ||
| groupParts = append(groupParts, fmt.Sprintf("\t%s = %s", spec.Name, spec.Value)) | ||
| } else { | ||
| groupParts = append(groupParts, fmt.Sprintf("\t%s %s", spec.Name, spec.Type)) | ||
| } |
There was a problem hiding this comment.
Render explicit const types when a value is present.
Typed constants like const Foo int = 1 will render as Foo = 1, dropping the explicit type.
Proposed fix
- if spec.Value != "" {
- groupParts = append(groupParts, fmt.Sprintf("\t%s = %s", spec.Name, spec.Value))
- } else {
- groupParts = append(groupParts, fmt.Sprintf("\t%s %s", spec.Name, spec.Type))
- }
+ switch {
+ case spec.Value != "" && spec.Type != "":
+ groupParts = append(groupParts, fmt.Sprintf("\t%s %s = %s", spec.Name, spec.Type, spec.Value))
+ case spec.Value != "":
+ groupParts = append(groupParts, fmt.Sprintf("\t%s = %s", spec.Name, spec.Value))
+ case spec.Type != "":
+ groupParts = append(groupParts, fmt.Sprintf("\t%s %s", spec.Name, spec.Type))
+ default:
+ groupParts = append(groupParts, fmt.Sprintf("\t%s", spec.Name))
+ }📝 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.
| for _, spec := range group.Specs { | |
| if !r.opts.ShouldRender(spec.Exported) { | |
| continue | |
| } | |
| if spec.Value != "" { | |
| groupParts = append(groupParts, fmt.Sprintf("\t%s = %s", spec.Name, spec.Value)) | |
| } else { | |
| groupParts = append(groupParts, fmt.Sprintf("\t%s %s", spec.Name, spec.Type)) | |
| } | |
| for _, spec := range group.Specs { | |
| if !r.opts.ShouldRender(spec.Exported) { | |
| continue | |
| } | |
| switch { | |
| case spec.Value != "" && spec.Type != "": | |
| groupParts = append(groupParts, fmt.Sprintf("\t%s %s = %s", spec.Name, spec.Type, spec.Value)) | |
| case spec.Value != "": | |
| groupParts = append(groupParts, fmt.Sprintf("\t%s = %s", spec.Name, spec.Value)) | |
| case spec.Type != "": | |
| groupParts = append(groupParts, fmt.Sprintf("\t%s %s", spec.Name, spec.Type)) | |
| default: | |
| groupParts = append(groupParts, fmt.Sprintf("\t%s", spec.Name)) | |
| } |
| for _, f := range typ.Fields { | ||
| if r.opts.ShouldRender(f.Exported) { | ||
| line := fmt.Sprintf("- `%s %s`", f.Name, f.Type) | ||
| if link := r.sourceLink(f.Pos); link != "" { | ||
| line = fmt.Sprintf("%s ([source](%s))", line, link) | ||
| } |
There was a problem hiding this comment.
Handle embedded fields without a name.
For embedded fields, f.Name is empty, so the output becomes ` io.Reader` with a leading space.
Proposed fix
- line := fmt.Sprintf("- `%s %s`", f.Name, f.Type)
+ var line string
+ if f.Name != "" {
+ line = fmt.Sprintf("- `%s %s`", f.Name, f.Type)
+ } else {
+ line = fmt.Sprintf("- `%s`", f.Type)
+ }| for _, param := range params { | ||
| desc := "" | ||
| if param.Name != "" { | ||
| desc = descByName[param.Name] | ||
| } | ||
| name := "" | ||
| if param.Name != "" { | ||
| name = "`" + escapeTableCell(param.Name) + "`" | ||
| } | ||
| lines = append(lines, fmt.Sprintf("| %s | %s | %s |", | ||
| name, | ||
| escapeTableCell(param.Type), | ||
| escapeTableCell(desc), | ||
| )) |
There was a problem hiding this comment.
Don’t drop descriptions for unnamed parameters.
When parameters are unnamed, descriptions keyed by type are ignored, so docs can disappear.
Proposed fix
- desc := ""
- if param.Name != "" {
- desc = descByName[param.Name]
- }
+ desc := ""
+ if param.Name != "" {
+ desc = descByName[param.Name]
+ } else if param.Type != "" {
+ desc = descByName[param.Type]
+ }📝 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.
| for _, param := range params { | |
| desc := "" | |
| if param.Name != "" { | |
| desc = descByName[param.Name] | |
| } | |
| name := "" | |
| if param.Name != "" { | |
| name = "`" + escapeTableCell(param.Name) + "`" | |
| } | |
| lines = append(lines, fmt.Sprintf("| %s | %s | %s |", | |
| name, | |
| escapeTableCell(param.Type), | |
| escapeTableCell(desc), | |
| )) | |
| for _, param := range params { | |
| desc := "" | |
| if param.Name != "" { | |
| desc = descByName[param.Name] | |
| } else if param.Type != "" { | |
| desc = descByName[param.Type] | |
| } | |
| name := "" | |
| if param.Name != "" { | |
| name = "`" + escapeTableCell(param.Name) + "`" | |
| } | |
| lines = append(lines, fmt.Sprintf("| %s | %s | %s |", | |
| name, | |
| escapeTableCell(param.Type), | |
| escapeTableCell(desc), | |
| )) |
|
I think it would be best to move this to a separate repository. |
3 participants
Description
This PR introduces the full gnodoc pipeline to generate Markdown documentation directly from the contract codebase. It provides a local, source-driven alternative to
gno doc, allowing documentation to be produced before deployment while preserving pkg.go.dev-style structure. The implementation covers parsing, modeling, rendering, and CLI behaviors so contracts can be documented consistently in development workflows.Implementation details
Markdown Output Example (Simplified)
Summary by CodeRabbit
New Features
Parser & Model
Documentation
Tests
Chore
✏️ Tip: You can customize this high-level summary in your review settings.