feat: modularise toc part generation with variable padding length - #369
feat: modularise toc part generation with variable padding length#369thompson-tomo wants to merge 8 commits into
Conversation
thompson-tomo
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors TOC assembly by extracting TOC header/title/footer line generation into lib/content-generation.js, aiming to improve modularity and enable configurable padding around TOC parts.
Changes:
- Moved TOC header/title/footer line construction into reusable helpers (
tocHeader,tocTitle,tocFooter) inlib/content-generation.js. - Updated
lib/transform.jsto use the new content generator helpers and to centralize title options underoptions.toc.title. - Adjusted
README.mddocumentation for TOC title padding.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| README.md | Updates documentation around --toc-title-padding-before behavior/expectations. |
| lib/transform.js | Switches TOC composition to use new content-generation helpers and updates title inference flow. |
| lib/content-generation.js | Introduces helpers to generate TOC parts with padding and content selection logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cad5716 to
080da6b
Compare
|
Just a heads up, I’ve been dealing with a serious family medical situation
for the past six weeks, I’ll try to catch up on reviews in the next week or
two. Thanks for your patience.
…--
Sent from a tiny screen; please excuse brevity or errors, or both.
On Fri, Jun 26, 2026 at 06:50 James Thompson ***@***.***> wrote:
@thompson-tomo <https://github.com/thompson-tomo> requested your review
on: thlorenz/doctoc#369 <#369>
feat: modularise toc part generation as a code owner.
—
Reply to this email directly, view it on GitHub
<#369?email_source=notifications&email_token=ABHSOTAZECBTLOP6PW5SBCL5BZIPXA5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTENZSGQZDKMZYHA3DJJTSMVQXG33OWBZGK5TJMV3V64TFOF2WK43UMVSKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#event-27242538864>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHSOTBI2GQIO7U6QXGQBCD5BZIPXAVCNFSNUABDKJSXA33TNF2G64TZHM2DINJZGM3DOO2JONZXKZJ3GQ3TKMBWGA3DONRQUF3AE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
No worries at all @AndrewSouthpaw hope that everyone is on the mend. 🤞 |
|
|
||
| function tocPart(padding, content){ | ||
| if (content === undefined) return []; | ||
| var lines = Array(Number(padding?.before ?? 0)).fill(''); |
There was a problem hiding this comment.
The CLI needs validation on non-negative integers, e.g.:
$ doctoc --toc-title-padding-before 0.5 README.md
RangeError: Invalid array length
at tocPart (lib/content-generation.js:83:15)
Similar for 1.5.
Let's add some CLI arg validation?
| if (inferredTitle) { tocLines.push(inferredTitle); } | ||
| tocLines.push(...contentGenerator.tocHeader(options.toc.header)); | ||
| tocLines.push(...contentGenerator.tocTitle(options.toc.title, inferredTitle)); | ||
| tocLines.push(''); |
There was a problem hiding this comment.
padding.after stacks on top of this hardcoded blank rather than replacing it, so the new knob is effectively off by one — { toc: { title: { padding: { after: 1 } } } } produces two blank lines:
**Table of Contents** *generated with [DocToc](...)*
← padding.after
← this line
- [A](#a)
Similar to footer.
Can we add tests to cover this?
There was a problem hiding this comment.
That is expected. The hard coded padding is for the items. It is no different to enabling padding after header & before title.
| } | ||
|
|
||
| function tocHeader(headerOptions) { | ||
| return tocPart(headerOptions?.padding, headerOptions?.content); |
There was a problem hiding this comment.
header.padding and footer.padding are read here but nothing ever sets them — no flag in doctoc.js, no mention in the README, no test. Same for title.padding.after. Only title.padding.before has a way in.
Either wire up the flags and docs in this PR (which is what the title suggests), or leave padding off header/footer until there's a way to reach it. As-is it reads like a feature but is dead for every CLI user.
| } | ||
|
|
||
| function tocTitle(titleOptions, existingTitle) { | ||
| if (titleOptions?.remove || (existingTitle === "" && !titleOptions?.content)) return []; |
There was a problem hiding this comment.
This creates a with determineTitle that nothing spells out: undefined means "nothing known, use the default", "" means "the existing TOC deliberately had no title, keep it that way", and a non-empty string means "preserve this one". Maybe worth a simple object, like:
const titleSetting: {
default: undefined,
existingTocNoTitle: "",
preserveTocTitle: "...",
}
Or at least some comments.
| excludeTag, | ||
| tocFooter, | ||
| tocHeader, | ||
| tocTitle |
There was a problem hiding this comment.
General thought: add direct unit tests for tocTitle's fallback chain (explicit content → existing title → default → removed).
Remove restriction on padding before title greater than 1.
This extracts the generation of the toc parts into the content generator module.
This extraction improves testability, functionality scoping & lays the foundation for enabling full control of padding before & after the content.
This approach enables control of the qty of padding before title etc.