Skip to content

feat: Add --insert flag to enable in-place TOC insertion#57

Open
ryo246912 wants to merge 2 commits into
ekalinin:masterfrom
ryo246912:feature/insert
Open

feat: Add --insert flag to enable in-place TOC insertion#57
ryo246912 wants to merge 2 commits into
ekalinin:masterfrom
ryo246912:feature/insert

Conversation

@ryo246912

@ryo246912 ryo246912 commented Feb 7, 2026

Copy link
Copy Markdown

Fix: #26

This PR implements the --insert flag feature that allows users to insert generated TOC directly into markdown files, similar to the bash version of github-markdown-toc and doctoc.

Supported Commands

# Insert TOC into a file (creates backup by default)
gh-md-toc --insert README.md

# Insert TOC without creating backup
gh-md-toc --insert --no-backup README.md

@ekalinin

ekalinin commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR @ryo246912, this is a nicely structured feature.
A few things I'd like to see addressed before merge.

Must fix

  • Tests. There are no *_test.go files in the PR, and the two new adapters (filebackup.go, tocinserter.go) hold the most bug-prone logic (marker validation, replaceContent, insertAtTop). CONTRIBUTING.md sets adapters at >95% coverage, so could you add table-driven tests using t.TempDir()? Cases worth covering: markers present / absent / mismatched / inline, header and footer on/off, first-heading vs no-heading, and backup content + permissions.
  • Non-atomic in-place write. InsertToc uses os.WriteFile, which truncates then writes. If the process is interrupted mid-write with --no-backup, the original file is lost with no recovery. Please write to a temp file in the same directory and os.Rename over the original (atomic on POSIX), keeping the backup as a secondary safeguard.

Should fix

  • Marker detection vs replacement mismatch. hasMarkers / validateMarkers match with strings.Contains / strings.Count, but replaceContent only matches whole lines (trimmed == MarkerStart). A file with inline markers such as <!--ts--> old <!--te--> passes validation, then falls through replaceContent and returns a misleading "markers not found" error, failing the whole run. Please match consistently, or validate that markers must be on their own line.
  • --insert with STDIN. STDIN is written to a temp file whose type is TypeLocalMD, so the insert path runs on it: it edits a file that is deleted right after, and CreateBackup leaves an orphaned ghtocXXXX.<timestamp> in the temp dir that the defer os.Remove does not clean up. Insert should be skipped for STDIN, the same way it is already skipped for non-local URLs in file.go.

Minor / nits

  • validateMarkers: the if startCount == 0 && endCount == 0 { return nil } branch is dead code, since the function returns nil on the next line anyway.
  • filebackup.go: os.Create uses default permissions instead of copying the source mode, and the second-resolution timestamp means two runs within the same second overwrite the earlier backup. InsertToc already preserves stat.Mode().Perm(), so it would be nice to be consistent.
  • generateTimestamp embeds the username and current time, so re-running rewrites the file (and makes a new backup) even when the TOC is unchanged. This mirrors the bash tool, so it may be intended, just flagging it.
  • insertAtTop treats any line starting with # as a heading, so a # comment inside a leading fenced code block would be picked as the insertion point. Low impact.
  • Toc.String() uses a pointer receiver while At uses a value receiver, and it duplicates the row-join logic already in CustomPrint. Minor consistency point.

Overall a solid contribution that fits the architecture well. The tests and the atomic write are the main blockers for me. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add --insert option (like in shell project)

2 participants