Use Markdown for installer and manual docs#24
Conversation
Code Review: [codex] Use Markdown for installer and manual docsOverviewThis PR replaces binary/RTF installer documents and the LaTeX/PDF manual placeholder with Markdown sources, generating installer-compatible RTF (macOS) and plain TXT (Windows) files at build time via a new prepare_installer_docs.py script. The approach is a clear maintainability win: Markdown is diff-friendly, human-readable, and a much lower barrier for plugin developers to customise their docs. Positives
Issues and Suggestions1. PowerShell CI step lacks error propagation (minor)In .github/workflows/build-win.yml, the Validate step runs python scripts/prepare_installer_docs.py win inside pwsh. PowerShell does NOT stop on non-zero exit codes from external processes by default. If the script fails, the Test-Path checks that follow will catch missing files -- but a Python traceback could be swallowed and make debugging harder. Suggestion: add $ErrorActionPreference = 'Stop' as the first line of that run block. 2. .md manual installed as end-user artefact (UX concern)The Inno Setup .iss and makedist-mac.sh now ship "TemplateProject manual.md" directly to end users and create a Start Menu shortcut pointing to it. On Windows, .md has no default file association on most machines -- double-clicking the shortcut will either prompt "Open with..." or silently fail. PDF was user-friendly here. Options to consider:
3. Hardcoded PRODUCT_NAME in prepare_installer_docs.py (minor)PRODUCT_NAME = "TemplateProject" (line 12) is only used for the optional installer background image filename. When duplicate.py creates a new plugin, if it only renames files/directories without patching all string occurrences inside script bodies, this constant could be missed. Consider deriving it at runtime: PRODUCT_NAME = os.path.basename(PROJECT_DIR). 4. Italic regex can match mid-word underscores (minor)The regex r"([^_]+)" will incorrectly strip underscores from snake_case identifiers or file paths in installer text. If a plugin developer writes a license or readme with technical paths or variable names, the text could be silently corrupted. A comment noting this limitation would help. 5. Setext-style headings not handledThe converter only strips ATX headings (# Heading). CommonMark also supports setext headings (text underlined with === or ---). Not a problem with the current .md files, but worth documenting as a known limitation in the script. 6. Missing newline at EOF additions confirmedSeveral files previously missing a trailing newline (TemplateProject.iss, makedist-win.bat, makeinstaller-mac.sh) have it added here. Good hygiene. SummaryThe core approach is solid and the implementation is clean. The two things most worth addressing before merge are the PowerShell error propagation (an easy one-liner fix) and the end-user UX of shipping a raw .md file with a Start Menu shortcut (a product decision about what format end users should receive). The rest are minor polish items. Review generated with Claude Sonnet 4.6 |
Code Review — PR #24: Use Markdown for installer and manual docsOverviewThis PR replaces binary/proprietary RTF and LaTeX/PDF installer sources with human-readable Markdown files, generating the installer-facing RTF/TXT files at packaging time via a new Python helper. It's a well-motivated change that reduces merge conflicts and makes docs editable without specialist tools. The approach is sound and the implementation is generally clean. Bugs / Issues1. Windows CI step may silently swallow Python errors - name: Validate installer docs
shell: pwsh
run: |
cd ${{matrix.project}}
python scripts/prepare_installer_docs.py win # ← no exit-code guard
if (!(Test-Path "build-win\installer-docs\license.txt")) { throw "..." }Unlike the macOS step (which has python scripts/prepare_installer_docs.py win
if ($LASTEXITCODE -ne 0) { exit 1 }Or set 2. Shipping a ; TemplateProject.iss
Source: "..\manual\TemplateProject manual.md"; DestDir: "{app}"
Name: "{group}\User guide"; Filename: "{app}\TemplateProject manual.md"Windows has no default handler for 3. PRODUCT_NAME = "TemplateProject"This constant is used only for the background image lookup. When
Code Quality Notes4. RTF font table missing family designation r"{\fonttbl{\f0 Helvetica;}}",The RTF spec recommends a font family keyword for reliable fallback rendering. Prefer: r"{\fonttbl{\f0\fswiss Helvetica;}}",This is cosmetic on macOS but good practice. 5. The converter covers ATX headings, bold, italic, inline code, inline links, and unordered lists. It does not handle:
For the current Markdown sources (which are all simple), this is fine. A brief docstring noting the intentional scope would prevent future contributors from adding unsupported syntax and wondering why output looks wrong. 6. elif codepoint <= 0x7f:
escaped.append(char)A What's Done Well
SummaryThe three items to address before merging:
The rest are minor polish items. Overall this is a clean improvement to developer ergonomics. |
Code Review — PR #24: Use Markdown for installer and manual docsOverall this is a well-motivated improvement: Markdown is much easier to maintain than hand-edited RTF/LaTeX, and the generated approach is clean. A few issues are worth addressing before merging. Positive Changes
IssuesPotential Bug — Windows TXT line endings
with open(path, "w", encoding="utf-8", newline="\n") as output_file:Unix line endings ( Potential Bug — Underscore italic regex over-matches
line = re.sub(r"_([^_]+)_", r"\1", line)This will strip underscores from line = re.sub(r"(?<!\w)_([^_]+)_(?!\w)", r"\1", line)Minor — Unrelated changes bundled in PRTwo changes appear unrelated to the Markdown migration:
Both are good fixes but worth calling out explicitly in the PR description (or splitting into a separate PR) so reviewers can audit them independently. Minor — Unquoted shell variables
rm -r $RESOURCES_DIRThe new variable rm -r "$RESOURCES_DIR"Minor —
|
| Severity | Issue |
|---|---|
| Should fix | Windows .txt files written with \n — may display incorrectly in Inno Setup wizard |
| Should fix | _underscore_ italic regex matches inside snake_case words |
| Nice to have | Quote $RESOURCES_DIR and $PKG_DIR in shell scripts |
| Informational | Unrelated fixes bundled (CLAP filename, GITHUB_OUTPUT) — fine to keep but worth noting |
| Informational | Markdown converter misses setext headers, blockquotes, HRs |
The core approach is sound. Addressing the line-ending issue and the underscore regex would make this production-ready.
Code ReviewOverviewThis PR is a solid quality-of-life improvement: switching binary/opaque RTF/LaTeX/PDF files to Markdown as the source of truth for installer docs and the user manual. The CI validation additions are a nice touch. A few things worth discussing before merging. Bugs / Issues1. In line = re.sub(r"_([^_]+)_", r"\1", line)will incorrectly strip underscores from words like 2. def pdf_text(text):
text = text.encode("cp1252", "replace").decode("cp1252")Characters outside the CP1252 range are silently replaced with 3. PRODUCT_NAME = "TemplateProject"When 4. The match changed from: if 'Source: "readme' in line:to: if 'DestName: "readme.txt"' in line:The new pattern works for the current template but will silently skip the substitution if the Moderate Concerns5. Hand-rolled PDF generator
6. if os.path.exists(background):
os.makedirs(target_dir, exist_ok=True) # ← inside the if
shutil.copy2(background, ...)
Minor Points
What's Working Well
Overall this is a good direction. Addressing the |
Code Review: Use Markdown for installer and manual docsOverview: This PR replaces binary/compiled distribution artefacts (RTF, PDF, LaTeX) with Markdown sources and a Python script to generate installer-ready formats at build time. It also bundles several unrelated bug fixes. Bug Fixes (three real ones, worth calling out)
prepare_installer_docs.pyClean and well-structured overall. A few observations: Minor — Minor — Key question — does CI WorkflowsAction version pinning. Both Redundant check (macOS validate step). update_installer-win.pyThe regex-based detection of the readme entry replaces a fragile substring check and is a clear improvement. The two-regex approach (source line shape + DestName sentinel) is precise and readable. Shell Scripts — makeinstaller-mac.shThe systematic quoting of all variable expansions is correct and overdue. Cleanup of Content Notes
SummarySolid work. The Markdown-first approach is the right call for maintainability, and the three embedded bug fixes are real improvements that benefit any project derived from this template. The main open question before merging: does |
Code Review: Use Markdown for installer and manual docsOverall this is a well-executed improvement. Switching from binary RTF/LaTeX to editable Markdown dramatically lowers the barrier for updating installer text and documentation. The CI validation, CRLF handling, and shell quoting work are all solid. A few things worth addressing: Bugs fixed (worth calling out)
Issues1. CI action version pinning (security) uses: pandoc/actions/setup@v1
uses: typst-community/setup-typst@v5Both use floating version tags rather than SHA pins. A malicious push to those tags would silently run arbitrary code in CI with full repository access. Recommend pinning to specific commit SHAs: uses: pandoc/actions/setup@v1.0.0 # replace with sha: ...
uses: typst-community/setup-typst@v5.0.0Or at minimum document why floating tags are acceptable here. 2. def write_text(path, text, newline="\n"):
os.makedirs(os.path.dirname(path), exist_ok=True)If parent = os.path.dirname(path)
if parent:
os.makedirs(parent, exist_ok=True)3. Temp file left behind on def pandoc_plain_text(source_path, target_path, newline="\n"):
temp_path = target_path + ".tmp"
pandoc_convert(source_path, temp_path, "plain", ["--wrap=none"])
try:
...
finally:
if os.path.exists(temp_path):
os.remove(temp_path)If try:
pandoc_convert(source_path, temp_path, "plain", ["--wrap=none"])
text = read_text(temp_path).strip() + "\n"
write_text(target_path, text, newline=newline)
finally:
if os.path.exists(temp_path):
os.remove(temp_path)4. Redundant def build_manual_pdf(build_dir_name):
require_tool("typst") # checked here
...
run_pandoc([...]) # run_pandoc -> require_tool("pandoc") checked here
Suggestions
The generated files under
The original Regex in The new What's good
Summary: Address the action SHA pinning (security) and the |
Summary
This switches the TemplateProject installer text and manual sources to Markdown.
Pandoc is now the renderer for generated distribution files:
installer/*.mdinstaller/*.mdwith CRLF line endingsmanual/TemplateProject manual.mdusing Pandoc with Typst as the PDF engineThe checked-in LaTeX/PDF manual placeholder is replaced by Markdown source. Packaging keeps generated RTF/TXT/PDF output under build directories and includes the generated PDF manual in both the macOS DMG and Windows installer/archive.
Review feedback addressed
prepare_installer_docs.pyderives the product name from the project directory instead of hardcodingTemplateProject.CI and build impact
build-mac/installer/resourcesbeforeproductbuild.build-mac/manual/TemplateProject manual.pdfand copies that PDF into the DMG folder.build-win/installer-docsbefore running Inno Setup.build-win/manual/TemplateProject manual.pdfand includes that PDF in the installer/archive.Release validation
v0.0.0-test5Release Native workflow passed: https://github.com/iPlug2/iPlug2OOS/actions/runs/25990651803TemplateProject-v0.0.0-mac.dmgTemplateProject-v0.0.0-mac-dSYMs.zipTemplateProject-v0.0.0-win.zipTemplateProject-v0.0.0-win-pdbs.zipLocal validation
python3 TemplateProject/scripts/prepare_installer_docs.py allfile 'TemplateProject/build-mac/manual/TemplateProject manual.pdf' 'TemplateProject/build-win/manual/TemplateProject manual.pdf' TemplateProject/build-mac/installer/resources/license.rtf TemplateProject/build-win/installer-docs/readme-win.txtpython3 -m py_compile TemplateProject/scripts/prepare_installer_docs.py TemplateProject/scripts/update_installer-win.py TemplateProject/scripts/makezip-win.pybash -n TemplateProject/scripts/makeinstaller-mac.shbash -n TemplateProject/scripts/makedist-mac.shgit diff --check