autodoc: ensure unique example storage with duplicate titles#71
autodoc: ensure unique example storage with duplicate titles#71becky-gilbert wants to merge 11 commits into
autodoc: ensure unique example storage with duplicate titles#71Conversation
…: getExampleInfo now just returns info object (no top-level file/example key) and adds hasCustomTitle flag; collectExamples keys example info by the file path (always unique), sets display path for title (default titles only), checks for and handles duplicate titles (warning, adds number to prevent overwritting), puts everything in try/catch
…e keyed by file path (always unique), adds title, hasCustomTitle, and displayPath to example value object
…d tests for title
…with default example titles, not custom
…or same title and file name but different paths
…ecause paths are included and unique
|
…ath rather than title as key for accessing stored examples
…le, and only skip file if examplesPath is a directory
…ss of directory structure)
autodoc: ensure unique example section namesautodoc: ensure unique example storage with duplicate titles
jadeddelta
left a comment
There was a problem hiding this comment.
just some things to address with handling different OS file paths
| ); | ||
| isDirectory = true; | ||
| const collectHtml = (dir: string) => { | ||
| for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { |
There was a problem hiding this comment.
from what I understand, fs.readdirSync is unsorted (implementation differs across OSes) so whichever one of shared titles becoming the secondary one might get flipped. would be good to sort the array before looping over it
| try { | ||
| const info = getExampleInfo(file, inferFallback); | ||
| if (info) { | ||
| info.displayPath = isDirectory ? path.relative(examplePath, info.path) : path.basename(info.path); |
There was a problem hiding this comment.
not sure if this ensures if paths could persist across OSes, thinking about windows using backslash (darn windows..), consider either using relativizeExamplePaths in the CLI utils.ts, or .split(path.sep).join("/") and then remove the relativizeExamplePaths call in the runCli function in cli.ts.
| const baseTitle = info.title; | ||
| const count = titleCounts.get(baseTitle) ?? 0; | ||
| titleCounts.set(baseTitle, count + 1); | ||
| if (count > 0) { |
There was a problem hiding this comment.
it looks like this might not work for titles "Hello", "Hello (2)", "Hello", the paths are stored separately so it won't be clobbered but it will result in duplicate section headers once rendered out. not sure if worth doing this edge case because it's not even that big of a deal, just flagging it
Fixes #61
This PR contains changes to address the problem of multiple examples having the same title and overwriting one another when collected, which was caused by keying the examples by title (which may collide) in the example storage data structure. This PR solves the issue by using the example file path (always unique) as the example identifier/key.
There are some other changes here that are related to the core bug fix but definitely subjective, so just let me know if you want me to remove any of those things.
Source code
src/parsers/utils.tscollectExamplesnow keys the examples by the file path returned bygetExampleInfo, which is always unique, instead of its title, which could collide. And the value object for these examples now includestitle,hasCustomTitle, anddisplayPathalongsidepathandcode(see related changes toExampleInfotype below).getExampleInforeturn type changed from:Record<string, ExampleInfo> | undefinedto
ExampleInfo | undefinedso that one file always produces exactly one example object result. (Previously,
getExampleInfoalways returned an object with exactly one key-value pair for a single example file, so the Record wrapper was unnecessary. The key-value handling for multiple examples is handled bycollectExamples).collectExamplesnow scans directories recursively (previously flat only)displayPathis computed as path relative to the examples root for directory scans, or just the filename for single-file input<title>tag titles are left as-is because thedisplayPathin the heading handles uniqueness at render time.collectExamplesfile iteration and title logic intry/catchso that a single bad example file (e.g. no title tag or sentinel) in a directory doesn't crash the whole run. (If the example path is a single file rather than a directory, it will still throw as before.)src/renderers/extension.ts/plugin.ts/timeline.ts### ${title} (${path})(always show file path)to
### ${title}(no path whenhasCustomTitleis true), or### ${title} (${displayPath})(same as original whenhasCustomTitleis false)This way, custom sentinel titles are treated as intentional and complete, and the user can use this method to remove the file path from the example's section title. Whereas the use of the
<title>tag is treated as the default case and not always expected to be unique. (Note that duplicate sentinel titles are still handled but that's done by the parser)src/types/info.tsAdds the following to
ExampleInfotype:title: moved out of the record key into the valuehasCustomTitle: whether the title came from thejspsych-autodoc:titlesentinel (vs<title>tag)displayPath: the path shown in section headings, relative to the examples root directoryTests
New tests and test files
tests/parsers/extension.test.tsdisplayPathvalues include the subdirectory to ensure uniqueness.tests/renderers/extension.test.tsNew fixture files
title-sentinel-collision-testswithfile-a.htmlandfile-b.html- both with the same jspsych-autodoc:title sentineltitle-tag-collision-testswithfile-a.htmlandfile-b.html- both with the same <title> tagtitle-filepath-collision-testswith/path1/duplicated_filename.htmlandpath2/duplicated_filename.html- same filename with <title> tags in different subdirectoriesChanges to existing tests and test files
Parser tests
Renderer tests:
hasCustomTitle: trueso the path no longer appears in the rendered heading)Renderer fixtures (info objects):
Snapshots