Refactor reusable YAML, markdown, and HTML builders#311
Conversation
PR Review — Refactor reusable YAML, Markdown, and HTML buildersThis is a well-motivated refactor. The extracted builders are clean, the IDisposable indent-scope pattern is consistent, and HTML encoding is applied correctly throughout. A few things worth addressing before merging: BugsDuplicate null check in // Lines added in WebApiDocsGenerator.ApiDocs.NavTokens.cs
if (item is null || string.IsNullOrWhiteSpace(item.Text))
return string.Empty;
if (string.IsNullOrWhiteSpace(item.Text)) // ← unreachable, always false here
return string.Empty;The second guard is redundant — it can never fire. Same pattern appears in the Inconsistent API between the two Markdown builders
Related: Minor issuesUnnecessary
// current
return JoinHtmlFragments(items.Select(BuildNavItemHtml).ToArray());
return JoinHtmlFragments(fragments.ToArray());
// simpler
return string.Concat(items.Select(BuildNavItemHtml));
return string.Concat(fragments);
Each scalar field in front matter creates a new writer, builds one line, converts to string, and discards the writer. Since
The Test coverage gaps
Positive notes
Overall this is a solid cleanup. The duplicate-check bug and the |
PR Review: Refactor reusable YAML, markdown, and HTML buildersOverall this is a well-structured refactoring. The new builder classes have clean APIs, the Potential bug: The line Minor:
Minor:
Code quality: Each Code quality: attribute list allocation in The refactored nav item renderer allocates a Missing tests:
Nit: The escape check list includes the hyphen character, which causes any value with a hyphen (e.g. Nit: Standard Markdown tables support |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21457e90d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static string Normalize(string text) | ||
| => (text ?? string.Empty).Replace("\r\n", "\n").Replace('\r', '\n').Trim().Replace("\n", Environment.NewLine); |
There was a problem hiding this comment.
Preserve leading whitespace in fenced code blocks
CodeFence normalizes content with Trim(), which removes leading indentation and leading blank lines from snippets before writing them. MarkdownHelpWriter now sends syntax/examples through this path, so indented examples (for example nested YAML/PowerShell blocks or here-string bodies) get rewritten and may no longer be copy-paste correct. Prefer end-only trimming for fenced code so original code indentation is preserved.
Useful? React with 👍 / 👎.
PR Review: Refactor reusable YAML, markdown, and HTML buildersOverall this is a well-structured refactor. The new builder classes have clear responsibilities, consistent patterns, good argument validation, and focused tests. The dual-signature migration approach (keeping Issues
table.AddRow("x | y", "12");
// produces: | x | y | 12 | — parsed as 3 columnsIf callers are always trusted to pre-escape pipes, that should be stated in a code comment. Otherwise,
The EscapeScalar("my-package") // → "my-package" (quoted)YAML only requires quoting Minor observationsDuplicated
private static string Normalize(string text)
=> (text ?? string.Empty).Replace("\r\n", "\n").Replace('\r', '\n').Trim().Replace("\n", Environment.NewLine);Both are
Each
GFM supports What's working well
🤖 Generated with Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fc8cf9fc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| html.AppendRaw(System.Web.HttpUtility.HtmlEncode(normalizedCode)); | ||
| html.Line("</code></pre>"); |
There was a problem hiding this comment.
Insert newline before closing example code tags
When an example code snippet does not already end with a newline (a common case for single-line examples), AppendRaw writes the encoded code and Line("</code></pre>") immediately appends the closing tags on the same line, so the builder’s indentation spaces become part of the rendered code text. This changes copy/paste output by introducing trailing spaces on the final code line, which is a regression from the previous behavior where closing tags were always emitted on their own line.
Useful? React with 👍 / 👎.
PR Review: Refactor reusable YAML, markdown, and HTML buildersGood refactor overall. The extraction of Potential bugs1. var installerType = installers[0].InstallerType; // could be null/empty
// ...
writer.WriteScalar("InstallerType", installerType); // writes an empty scalarIf 2. return normalized.StartsWith("-", StringComparison.Ordinal)
|| normalized.IndexOfAny(new[] { ':', '#', ... }) >= 0
|| normalized.Contains(' ')
? "\"" + ... + "\""
: normalized;A bare 3. private static string EscapeCell(string? value)
=> (value ?? string.Empty).Replace("|", "\\|");A cell value like => (value ?? string.Empty).Replace("\\", "\\\\").Replace("|", "\\|");The existing pipe-escape test will still pass; the new edge case is Design / API observations4. Divergent blank-line behaviour between
5. public void FrontMatter(string key, string value)
{
var yaml = new YamlTextWriter();
yaml.WriteScalar(key, value);
_frontMatter.Append(yaml.ToString());
_hasFrontMatter = true;
}Each call allocates a Minor / nitpick6. Several helpers build HTML attributes via 7.
What's working well
|
Code Review: Refactor reusable YAML, markdown, and HTML buildersThe overall direction of this PR is excellent — extracting dedicated builder types reduces hand-rolled string assembly, centralises escaping logic, and makes the output contracts testable. The new classes are well-shaped and the test coverage for the core builder behaviours is solid. A few things to address before merging: Bug: Double-escaping in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a11f5bbcd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| markdown.AppendLine($"| {EscapeCell(item.Name)} | {FormatGiB(item.SizeInBytes)} | {FormatDate(item.CreatedAt)} | {FormatDate(item.UpdatedAt)} | {EscapeCell(item.Reason)} | {EscapeCell(DeleteState(item.DeleteStatusCode, item.DeleteError))} |"); | ||
| table.AddRow( | ||
| EscapeCell(item.Name), |
There was a problem hiding this comment.
Pass raw artifact fields into MarkdownTableBuilder
MarkdownTableBuilder.AddRow already escapes markdown table metacharacters, but this loop pre-escapes item.Name, item.Reason, and delete status via EscapeCell(...) before calling AddRow. When a value contains | or \, the output is double-escaped (for example a\\\|b), so rendered housekeeping reports show extra backslashes and differ from prior output. Keep newline normalization if needed, but avoid escaping these fields before passing them to the table builder.
Useful? React with 👍 / 👎.
Agent Guide (PSPublishModule / PowerForge.Web + Websites)Last updated: 2026-03-29 This file is the "start here" context for any agent working on the PowerForge.Web engine and the tracked sibling repos that build on it. Repos / Local PathsThese are the maintainer's default Windows paths (safe to assume in this workspace):
Portable Path Discovery (WSL/macOS/Linux)If you're not on Windows or you don't have
Practical search strategy:
Recommended environment variable (makes site scripts deterministic):
What To Read First (Canonical)
Reference docs (as needed):
Repo Skills (.agents/skills)This repo ships agent skills under
Working Agreements (Best Practices)
Module LayeringWhen touching the PowerShell module stack, prefer this boundary:
Quick smell test before adding code to a cmdlet:
If the answer to any of those is yes, the code probably belongs in Stop extracting when the remaining code is only:
Preferred pattern for the last 10-20%:
Quality Gates (Pattern)Each website should have:
Commands (Engine)
Commands (PowerShell Module)
Commands (Websites)From a website repo folder:
Baselines:
Current State (As Of 2026-02-09)
|
|
test3 ignore this comment |
PSPublishModule Changelog2.0.19 - 2025.06.17What's Changed
Full Changelog: v2.0.18...v2.0.19 2.0.18 - 2025.06.06What's new
2.0.17 - 2025.05.28
Get-ProjectVersion -Path "C:\Support\GitHub\DnsClientX" -ExcludeFolders @('C:\Support\GitHub\DnsClientX\Module\Artefacts')
Set-ProjectVersion -Path "C:\Support\GitHub\DnsClientX" -NewVersion "0.4.0"
Set-ProjectVersion -Path "C:\Support\GitHub\DnsClientX" -VersionType Build2.0.16 - 2024.10.27
2.0.15 - 2024.10.11What's Changed
New Contributors
Full Changelog: v2.0.14...v2.0.15 2.0.14 - 2024.08.14What's Changed
Full Changelog: v2.0.13...v2.0.14 2.0.13 - 2024.07.13
2.0.12 - 2024.05.06
2.0.11
2.0.10
2.0.9
2.0.8
2.0.7
2.0.6
2.0.5What's new
Full Changelog: v2.0.4...v2.0.5 2.0.4What's Changed
New Contributors
Full Changelog: v2.0.3...v2.0.4 2.0.3
2.0.2 - 2023.08.06
2.0.1 - 2023.08.04
2.0.0 - 2023.08.03
1.8.0-Preview5
1.8.0-Preview4
1.8.0-Preview3
1.8.0-Preview2
1.8.0-Preview1
1.7.0
1.6.0 - 2023.07.11
1.5.1 - 2023.07.06
1.5.0 - 2023.07.04
1.4.0 - 2023.07.03
1.3.0 - 2023.07.03
1.2.0 - 2023.06.23
1.1.0 - 2023.06.19
1.0.0 - 2023.06.18
0.9.53 - 2022.12.31
0.9.52 - 2022.12.30
I've removed the logic where Standard would always get loaded even if Default/Core was also provided which could lead to unexpected behavior.
0.9.51 - 2022.09.26
0.9.43 - 2022.04.14
|
Code Review: Refactor reusable YAML, markdown, and HTML builders\n\nGood refactoring overall. |
|
line1 |
Agent Guide (PSPublishModule / PowerForge.Web + Websites)Last updated: 2026-03-29 This file is the "start here" context for any agent working on the PowerForge.Web engine and the tracked sibling repos that build on it. Repos / Local PathsThese are the maintainer's default Windows paths (safe to assume in this workspace):
Portable Path Discovery (WSL/macOS/Linux)If you're not on Windows or you don't have
Practical search strategy:
Recommended environment variable (makes site scripts deterministic):
What To Read First (Canonical)
Reference docs (as needed):
Repo Skills (.agents/skills)This repo ships agent skills under
Working Agreements (Best Practices)
Module LayeringWhen touching the PowerShell module stack, prefer this boundary:
Quick smell test before adding code to a cmdlet:
If the answer to any of those is yes, the code probably belongs in Stop extracting when the remaining code is only:
Preferred pattern for the last 10-20%:
Quality Gates (Pattern)Each website should have:
Commands (Engine)
Commands (PowerShell Module)
Commands (Websites)From a website repo folder:
Baselines:
Current State (As Of 2026-02-09)
|
|
line1 |
Code Review: Refactor reusable YAML, markdown, and HTML buildersGood refactoring overall. Extracting Bug / CorrectnessYAML scalar escaping misses common special-value cases
Suggested fix — add a reserved-word set and check it in private static readonly HashSet<string> _yamlReserved = new(StringComparer.OrdinalIgnoreCase)
{
"true", "false", "yes", "no", "on", "off", "null", "~", "---", "..."
};
// extend the escape condition:
|| _yamlReserved.Contains(normalized)Whitespace-only blank lines in
public void BlankLine() => _builder.AppendLine();Design
The two classes share
The front-matter methods each construct a fresh
The property defaults to Missing test coverage
Minor nits
Overall this is a well-scoped refactor with good test hygiene. The YAML escaping gap is the only correctness risk worth fixing before merge. |
PR Review: Refactor reusable YAML, markdown, and HTML buildersThis is a solid refactoring effort. The new builder abstractions ( Bugs / Correctness1. Dead code in
The second // First guard (correct):
if (installers is null || installers.Count == 0)
throw new InvalidOperationException(...);
var writer = new YamlTextWriter();
// ...
var firstInstaller = installers.FirstOrDefault();
if (firstInstaller is null) // ← unreachable
throw new InvalidOperationException(...);Either remove the second check, or replace 2. Subtle output change in
Old code placed Since Design / Code Quality3.
4.
5.
Values like Test Coverage6. No test for
7. Only two scenarios are tested: no-installer guard and blank-type defaulting. Consider adding a round-trip test that builds a multi-architecture manifest and asserts the full YAML structure, which would also catch the duplicate-null-check dead code (#1). Minor Notes
Overall, the refactoring is clean, the indented-builder pattern is a nice improvement over the previous append-heavy style, and the new tests provide meaningful coverage of the key contracts. The main items worth addressing before merge are the dead code (#1) and the unescaped |
|
test comment - please approve |
|
Replacing test comment with actual review - see next comment. |
Code Review - Part 1: Bugs and PerformanceBug 1 - Silent behavior change in GitHubDiscussionMarkdownBuilder: The old loop added a blank line before every discussion heading entry. The new code adds a blank line only before the first entry via isFirstDiscussionEntry. Subsequent entries get no blank line because MarkdownThreadBuilder.Heading does not append a trailing blank, unlike MarkdownDocumentBuilder.Heading. This silently removes visual separation between thread entries. Bug 2 - Unreachable guard in WingetManifestWriter.Build: The method opens with a guard that throws for empty/null installer lists, then ~15 lines later checks if firstInstaller is null after calling installers.FirstOrDefault(). Because the first guard already handles the empty-collection case, this second check is dead code. Either add null-element validation to the first guard, or remove the unreachable check. Perf 1 - YamlTextWriter.EscapeScalar allocates a char array on every call: The IndexOfAny call uses an inline char array literal, allocating a new array on every invocation. Promoting it to a static readonly field means the allocation happens once. Perf 2 - string.Concat with LINQ Select materialises every fragment as a separate string: Several refactored methods use string.Concat(collection.Select(BuildXxxHtml)). The HtmlFragmentBuilder introduced in this same PR keeps a single StringBuilder throughout and would be more consistent and efficient. |
Code Review - Part 2: Code Quality and Test GapsQuality 1 - MarkdownDocumentBuilder.FrontMatter creates a new YamlTextWriter per entry: Each call instantiates a YamlTextWriter, writes one value, calls ToString(), then appends the result into _frontMatter. A single YamlTextWriter field for the front matter section would be cleaner and remove the intermediate string allocation on every call. Quality 2 - Duplicated logic between MarkdownDocumentBuilder and MarkdownThreadBuilder: Both classes contain nearly identical Paragraph, BlankLine, Heading, and Normalize implementations. The only real difference is whether Heading appends a trailing blank line. A shared base class or static helper would keep the Normalize logic in sync and reduce copy-paste drift. Quality 3 - No bounds check in BuildFieldTable: The method calls table.AddRow(row[0], row[1]) without verifying that row has at least two elements. An IndexOutOfRangeException here does not point back to the caller. A guard with a clearer message would be easier to diagnose. Tests 1 - HtmlFragmentBuilder.AppendRaw has no test: The method intentionally skips indentation. A test verifying it does not prefix spaces when called inside an Indent() scope would pin that contract. Tests 2 - YamlTextWriter indent path is only tested indirectly: The Indent() + WriteScalar/WriteKey path is exercised implicitly via WingetManifestWriterTests. A direct test in YamlTextWriterTests for nested output would make the contract explicit at the unit level. Tests 3 - Refactored ScribanThemeHelpers HTML builders have no new tests: The helpers were changed from void/StringBuilder helpers to pure string-returning functions, which is exactly the shape that makes unit testing straightforward. Contract tests for edge cases such as empty items, items with children, and external links getting rel=noopener would protect against regressions. |
Code Review - Part 3: Minor Nits and SummaryNit 1: The long inline interpolated string in ScribanThemeHelpers.MenuTree is harder to review than the original chained appends. A local variable for the inner HTML would help readability. Nit 2: EscapeScalar quotes a lone dash but not a dash followed by a non-space character. This is correct per YAML 1.2 (a bare sequence indicator only when followed by a space), but a short comment would help readers who wonder if it is a bug. Nit 3: MarkdownFrontMatterWriter has a single caller in WebSiteScaffolder. Now that MarkdownDocumentBuilder handles front matter directly, consider whether this helper is still needed. Good work overall. The builder pattern is much cleaner than the inline StringBuilder chains it replaces, and the new test files are well-structured. The main items to address are the silent behavior change in the discussion thread output (Bug 1) and the unreachable null guard (Bug 2). The EscapeScalar char array allocation (Perf 1) is a quick one-line fix worth picking up before merge. |
Summary
Why
This branch reduces the amount of hand-stitched string output across
PowerForgeandPowerForge.Web, which makes the rendering code easier to read, safer to reuse, and simpler to extend without copying more append-heavy patterns.Validation
dotnet build .\\PowerForge.Web\\PowerForge.Web.csproj -c Releasedotnet test .\\PowerForge.Tests\\PowerForge.Tests.csproj -c Release --filter "FullyQualifiedName~WebApiDocsSocialMetaTests|FullyQualifiedName~WebApiDocsGeneratorPowerShellTests|FullyQualifiedName~WebApiDocsGeneratorSourceAndCssTests|FullyQualifiedName~WebApiDocsGeneratorContractTests|FullyQualifiedName~WebApiDocsGeneratorCrefTests|FullyQualifiedName~WebApiDocsGeneratorUsageTests|FullyQualifiedName~WebApiDocsGeneratorRelatedContentTests|FullyQualifiedName~WebPipelineRunnerApiDocsPreflightTests|FullyQualifiedName~WebApiDocsGeneratorSuiteTests|FullyQualifiedName~WebPipelineRunnerProjectApiDocsTests"dotnet test .\\PowerForge.Tests\\PowerForge.Tests.csproj -c Release --filter "FullyQualifiedName~ScribanPfNavigationHelpersTests|FullyQualifiedName~WebSiteLocalizationFeaturesTests"