Build a Markdown.Table instead of hand-rolling the markdown writer - #1008
Open
andreasnoack wants to merge 1 commit into
Open
Build a Markdown.Table instead of hand-rolling the markdown writer#1008andreasnoack wants to merge 1 commit into
andreasnoack wants to merge 1 commit into
Conversation
andreasnoack
force-pushed
the
an/coeftable-markdown-table
branch
from
August 21, 2026 12:15
877cced to
3636f83
Compare
`show(::IO, ::MIME"text/markdown", ::CoefTable)` reimplements the markdown table format by hand, and in doing so never escapes `|`. Any cell containing one therefore presents more cells than the alignment row, and the table stops parsing as a table at all — downstream packages hit this with the R-style `Pr(>|z|)` p-value header. Rather than escaping by hand, stop reimplementing the format: render the rows through Base's `print_matrix_row` as before — that is what produces the decimal alignment — and hand the padded strings to `Markdown.Table`. Escaping and the alignment rules then come from the stdlib. The hand-rolled writer was justified by a comment saying the Markdown stdlib "won't give us nice decimal alignment". It does not produce the alignment, but it preserves it: `padcells!` adds the same width to every cell in a column, so pre-padded equal-width cells keep their decimal points lined up. Costs: Markdown becomes a (stdlib) dependency, and the emitted spacing changes, so the two golden markdown tests get new expected output. Cell content is unchanged, as is the rendered HTML and the plain-text show method.
andreasnoack
force-pushed
the
an/coeftable-markdown-table
branch
from
August 24, 2026 13:36
3636f83 to
38dc069
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1007, rebased on it now that it has merged.
#1007 fixed the escaping bug by hand, as a minimal short-term fix. This removes the reason the bug was possible: it stops reimplementing the markdown table format altogether. The rows still go through Base's
print_matrix_row— that is what produces the decimal alignment — and the padded strings are handed toMarkdown.Table, so escaping and the alignment rules come from the stdlib rather than fromescape_markdown_pipes.The justification for hand-rolling does not hold
The method carries this comment, from #664:
The stdlib does not produce decimal alignment, but it preserves it:
padcells!adds the same width to every cell in a column, so pre-padded equal-width cells keep their decimal points lined up. Sinceprint_matrix_rowalready emits exactly those strings, the alignment survives:Decimal points at the same offset in every row, pipes escaped, and the result parses as a
Markdown.Table.Costs
Markdownmoves from the test-only dependency Escape pipes when writing a CoefTable as markdown #1007 added to a direct dependency (still a stdlib, but a new entry in[deps]/[compat], and it drops out of[extras]/[targets]).|:--- | -------------:|rather than|:---|--------------:|), so both golden markdown tests get new expected output. Cell content is identical, and the rendered HTML is unaffected, as is the plain-textshowmethod.One thing to decide separately
The hand-rolled writer selects the alignment marker with
j-1 in [ct.teststatcol; ct.pvalcol]whilejindexescolnms, so the test-statistic column never receives its left-align marker and the p-value column receives it via the shift. That looks like an off-by-one, but changing it would change output beyond this refactor, so it is carried over verbatim with a comment. Happy to fix it here or in a follow-up, whichever you prefer.#1007's regression test (a pipe in a header, a row name and a string cell; asserts equal cell counts per row and that the output parses as a
Markdown.Table) is kept as-is and still passes. Full test suite passes locally on the rebased branch.🤖 Generated with Claude Code