Skip to content

fix(docs): separate flag types into a column - #1068

Merged
mbevc1 merged 6 commits into
mainfrom
20260731_docs_types
Aug 1, 2026
Merged

fix(docs): separate flag types into a column#1068
mbevc1 merged 6 commits into
mainfrom
20260731_docs_types

Conversation

@mbevc1

@mbevc1mbevc1 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Follow-up review fixes on top of the flag type column change:

  • Regenerate the mintlify goldens instead of editing them by hand. They had
    lost the trailing blank line the generator emits; the test only passed
    because compareFileBytes normalises with strings.TrimSpace, so the goldens
    no longer reproduced generator output byte for byte.
  • Drop cobra's alignment padding from the flag cell. Markdown strips leading
    whitespace inside a table cell, so the padding only added noise to the
    generated MDX; the maxlen pass that used to need it is gone.
  • Build the rows with a single strings.Builder. The intermediate slice, the
    bytes.Buffer and the second loop existed only to support that alignment
    pass, and the bytes import goes with them.
  • Move the table header into FlagTableHeader next to the row builder, so the
    column count cannot drift between the header and the rows, and cover that
    contract with a test.
  • Assert whole rows in the default and deprecation tests. Both previously
    matched on a substring that would have passed had the text leaked into the
    type column, which is the behaviour the change is about.
  • Pin where the NoOptDefVal syntax lands. No Kosli flag uses a non-bool
    NoOptDefVal today, so that path moved into the type column untested.

Verified by regenerating all 94 command docs and checking that every flag row
is a well formed three column row.

Obsoletes #1067

Closes#450

Checklist

@claude

claudeBot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mbevc1's task in 2m 43s —— View job


Review — PR #1068: separate flag types into a column

  • Gathered context (diff, changed files, generator + test helpers)
  • Static analysis: go build ./... ✅, go vet ./internal/docgen/... ✅ (compiles tests too)
  • Reviewed code quality & correctness
  • Reviewed tests
  • Posted feedback

Verdict: ✅ Looks good — merge-ready

This is a clean, well-reasoned follow-up. Each commit maps to exactly one concern from the review thread, and the reasoning is captured in comments rather than lost.

On JonJagger's question (empty type cells) and @mbevc1's reply: the latest commit c108ce3e resolves it directly — the varname == "" fallback to flag.Value.Type() (helpers.go:62-64) now names booleans, so the previously-blank column reads bool. The regenerated goldens confirm it (-D, --dry-run | bool, -h, --help | bool). This is also pflag's own convention. 👍

What's done well:

  • Header/row cohesion — moving the header into the exported FlagTableHeader const next to CommandsInTable, then pinning it with TestCommandsInTableColumnCount (counts unescaped | in every row vs. the header), is exactly the right way to stop the column count from drifting. MintlifyFormatter is the only Formatter implementation, so there's no second header to keep in sync.
  • Ordering — computing the type fallback before the NoOptDefVal block so an optional value reads bool[=x] mirroring string[="x"], with TestCommandsInTableBoolOptionalValue / TestCommandsInTableOptionalValue pinning both paths.
  • Whole-row assertions — the default and deprecation tests now match the full | flag | type | description | row, so leakage of a default/deprecation notice into the type column would fail. That's the exact regression the change guards against; substring matches would have missed it.
  • Goldens regenerated, not hand-edited — restoring the trailing blank line the generator emits. Worth noting the tests wouldn't have caught the drift (normalize() in testHelpers.go:322 does strings.TrimSpace), so this keeps the goldens a byte-for-byte reproduction of generator output regardless. Good discipline.
  • Dropping the alignment padding, the intermediate slice, the bytes.Buffer, and the bytes import in favor of a single strings.Builder — markdown strips leading cell whitespace anyway, so this is pure noise removal.

One conscious-decision note (inline on helpers.go:64, non-blocking): -h, --help now shows | bool | in every command's flag table — that's now in all 94 generated docs, not just the two sampled goldens in this PR. It's consistent with the "always name the type" rule, so no change needed; just flagging the blast radius.

Nit (optional):FlagTableHeader is exported but every consumer (mintlify.go, the test) is in-package, so it could be unexported. Harmless either way.

No correctness, security, or performance concerns. The per-flag defaultZero slice allocation is negligible for doc generation and was an intentional locality refactor.

Note: I couldn't execute go test ./internal/docgen/... in this environment (command approval), but go vet compiles the test files and the logic/goldens check out by inspection.

@mbevc1mbevc1 changed the title 20260731 docs typesfix(docs): separate flag types into a columnJul 31, 2026
@mbevc1
mbevc1force-pushed the 20260731_docs_types branch 3 times, most recently from 0416e94 to aaf8d02CompareJuly 31, 2026 16:54
@mbevc1
mbevc1 enabled auto-merge (squash) July 31, 2026 16:54
@JonJagger

JonJagger commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

I notice that some flags have no entry in the type column.
Are these all boolean flags with a default?
Is it worth adding an entry for these?

@mbevc1

Copy link
Copy Markdown
ContributorAuthor

Fair point and Pr is following #450 where it asked for the type (if there is one), which is also pflag's own convention. Happy to add that in so it's clear what are the types and don't leave it to reader to assume.

Follow-up review fixes on top of the flag type column change:
- Regenerate the mintlify goldens instead of editing them by hand. They had
lost the trailing blank line the generator emits; the test only passed
because compareFileBytes normalises with strings.TrimSpace, so the goldens
no longer reproduced generator output byte for byte.
- Drop cobra's alignment padding from the flag cell. Markdown strips leading
whitespace inside a table cell, so the padding only added noise to the
generated MDX; the maxlen pass that used to need it is gone.
- Build the rows with a single `strings.Builder`. The intermediate slice, the
bytes.Buffer and the second loop existed only to support that alignment
pass, and the bytes import goes with them.
- Move the table header into `FlagTableHeader` next to the row builder, so the
column count cannot drift between the header and the rows, and cover that
contract with a test.
- Assert whole rows in the default and deprecation tests. Both previously
matched on a substring that would have passed had the text leaked into the
type column, which is the behaviour the change is about.
- Pin where the NoOptDefVal syntax lands. No Kosli flag uses a non-bool
NoOptDefVal today, so that path moved into the type column untested.
Verified by regenerating all 94 command docs and checking that every flag row
is a well formed three column row.
A literal | in a flag's type or description was read as a column separator,
silently splitting the row into more columns than the header declares. The old
two column table had the same exposure, so this is pre-existing rather than a
regression from the type column.
No flag help contains a pipe today. The pattern only appears in command Long
text (diffSnapshots.go, getSnapshot.go), which renders as prose rather than as
a table row, so no generated page is currently affected. All 94 command docs
come out byte for byte identical to before this change.
- Escape | as \| in every cell. GFM unescapes it inside a table cell before
inline parsing, so it survives even in a code span. The escaping lives in
the row builder rather than in escapeMintlifyProse because that escaper also
runs over prose, where a \| would render as a literal backslash. This is why
the two Long text placeholders keep their raw pipes.
- Let angleBracketPattern match placeholders whose pipes are already escaped.
Without this, a flag description containing <hours|days> would arrive as
<hours\|days>, stop matching, and leak a raw JSX-looking tag into the MDX
build. The rendered cell is now `hours\|days`, which is both table safe and
MDX safe.
- Give the column count test flags that carry a pipe in the description and in
a default value, counting unescaped separators only, so it fails on exactly
the bug described above. Verified: without the escaping it reports 7 and 5
separators against the header's 4.
Hoisting the list to a package-level var traded a local for mutable package
state and bought nothing: the local composite literal does not escape, so it is
stack allocated. Benchmarked over 60 flags, both shapes are identical at
13331 B/op and 290 allocs/op, with ns/op inside the noise.
A `[...]string` array would not have fixed the mutability either, since a
package-level var is writable whatever its element type, and slices.Contains
would then need an extra [:]. Reverting to the local drops the question
altogether and leaves this hunk untouched relative to main.
Generated docs and goldens are unchanged.
Tightening angleBracketPattern to a precise "token, optionally pipe-separated"
grammar looked like it would encode the intent better, but it inverts the
failure mode. Over every angle-bracket string in the repo the two patterns agree
exactly; they diverge only on malformed input such as <hours||days>, <hours|> or
<a\b>, and there the strict pattern does not match. A non-match is not neutral:
the placeholder is then left as a raw <...> for the MDX build to read as a JSX
tag, which is the failure the escaping exists to prevent. The loose class fails
safe, a strict one fails the build.
Rather than change the pattern, record why it is loose and add a case that
proves it. Verified: the new case fails against the strict pattern and passes
against the current one.
Generated docs and goldens are unchanged.
pflag.UnquoteUsage returns an empty type name for booleans, so 403 of the 1546
flag rows had a blank cell in a column headed Type. That reads as missing data
and left the reader to infer that the flag takes no value. Fall back to the
underlying pflag type name, so a boolean says bool.
The fallback is on the empty string rather than on the bool type specifically,
so any future pflag type that declines to name itself is covered too. It runs
before the NoOptDefVal block, so an optional value reads as bool[=x], matching
the existing string[="x"].
Regenerated all 94 command docs: no cell is empty now, every row is still a
well formed three column row, and comparing against the previous output, 403
rows had an empty type become bool while the other 1143 are untouched. No flag
name or description changed anywhere.
@mbevc1
mbevc1force-pushed the 20260731_docs_types branch from 5105ecd to c108ce3CompareAugust 1, 2026 12:47
Comment threadinternal/docgen/helpers.go
@mbevc1
mbevc1 merged commit d0e7ca7 into mainAug 1, 2026
20 checks passed
@mbevc1
mbevc1 deleted the 20260731_docs_types branch August 1, 2026 14:49
Sign up for freeto 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.

Small CLI docs tweak

2 participants

@mbevc1@JonJagger