Skip to content

ci: Document with a patched roxygen2 that keeps the sentence gap - #2906

Open
krlmlr wants to merge 3 commits into
claude/readme-rmdfrom
claude/roxygen2-fork
Open

ci: Document with a patched roxygen2 that keeps the sentence gap#2906
krlmlr wants to merge 3 commits into
claude/readme-rmdfrom
claude/roxygen2-fork

Conversation

@krlmlr

@krlmlr krlmlr commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Split out of #2903 so it can be decided on its own.

The problem

commonmark discards the whitespace a line break stands for. roxygen2 turns every softbreak into a bare newline, so the .Rd carries a line break with nothing after it and Rd2txt() renders it as a single space. Prose written one sentence per line therefore comes out single-spaced, while the same words wrapped to a fixed width with two spaces after the period keep the gap.

That makes the two styles mutually exclusive: splitting a sentence pair separated by two spaces silently drops one of them. In this package that is what forced the long single-line roxygen paragraphs, and why the earlier reformatting pass had to leave 26 places joined — undoing them would have kept the gap only at the price of lines of 1440 and 973 characters.

Only the text renderer is affected. Rd2HTML() and Rd2latex() differ by whitespace that HTML and TeX collapse — and LaTeX applies its own sentence spacing regardless. But text is what ?topic shows, so it is the version most people read.

The change

A new composite action, .github/workflows/roxygen2-fork, installs roxygen2 from upstream with the R/ part of krlmlr/roxygen2@f-sentence-spacing applied on top, and R-CMD-check.yaml runs it immediately before the Roxygenize step.

It shallow-clones upstream, fetches the branch, computes the diff against their merge base restricted to R/, and applies it with git apply --3way. Restricting to R/ means an unrelated upstream change to a test fixture cannot fail the documentation step. It aborts if the patch is empty or does not apply, rather than silently installing an unpatched build, and afterwards asserts that what it installed really carries the patch.

Config/roxygen2/version becomes 8.1.0.9100. The .9100 suffix distinguishes a patched build from upstream's own .9000 development builds; if upstream moves, the x.y.z part follows it and the suffix stays. DESCRIPTION is DCF and cannot carry a comment line — read.dcf() rejects one as malformed — so the explanation lives in a Config/cynkra/roxygen2 field.

Why it is a separate pull request

Taking it changes rendered help across the package. Leaving it means the reformatting below renders as the package renders today everywhere except where a sentence pair separated by two spaces was split across lines: tools::Rd2txt() over the 633 topics that branch regenerates finds 80 of them differing, each by that gap narrowing to a single space and by nothing else. Neither half needs the other to be correct, so they should be able to be decided separately.

man/ is not edited by hand here. The regenerated .Rd files are generated output, so CI roxygenises and commits them itself — they arrive on this branch as an automated commit rather than in the hand-written diff.

Upstream

The roxygen2 side is krlmlr/roxygen2#8, on the fork, not yet filed with r-lib. It is deliberately narrow: mid-sentence breaks are untouched, links are unaffected, and two guards keep it from firing wrongly — an abbreviation list so a line ending in e.g. or et al. does not gain a mid-sentence gap, and a fix for a determinism bug where a line starting with \doi{} or \code{} gained or lost its gap at random, because the test was reading roxygen2's own random placeholder for protected Rd tags.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WWhverMTZZKgEpUuTK117m


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 35426dc is merged into claude/readme-rmd:

  • ✔️as_adjacency_matrix: 182ms -> 182ms [-0.45%, +0.78%]
  • ✔️as_biadjacency_matrix: 205ms -> 204ms [-1.21%, +0.24%]
  • ✔️as_data_frame_both: 207ms -> 207ms [-1.87%, +2.26%]
  • ✔️as_long_data_frame: 166ms -> 166ms [-1.96%, +1.36%]
  • ✔️es_attr_filter: 192ms -> 191ms [-1.39%, +0.56%]
  • ❗🐌graph_from_adjacency_matrix: 220ms -> 224ms [+0.27%, +3.66%]
  • ✔️graph_from_data_frame: 223ms -> 222ms [-1.67%, +1.2%]
  • ✔️vs_attr_filter: 236ms -> 237ms [-0.57%, +2.19%]
  • ✔️vs_by_name: 215ms -> 214ms [-1.1%, +0.11%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
krlmlr force-pushed the claude/roxygen2-fork branch 2 times, most recently from 33adc59 to 840234c Compare September 13, 2026 18:50
@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 33adc59 is merged into claude/readme-rmd:

  • ✔️as_adjacency_matrix: 181ms -> 179ms [-1.73%, +0.05%]
  • ✔️as_biadjacency_matrix: 202ms -> 204ms [-1.36%, +3.1%]
  • ✔️as_data_frame_both: 201ms -> 201ms [-0.81%, +0.6%]
  • ✔️as_long_data_frame: 161ms -> 160ms [-1.94%, +1.27%]
  • ✔️es_attr_filter: 192ms -> 191ms [-2.27%, +1.69%]
  • ✔️graph_from_adjacency_matrix: 218ms -> 219ms [-0.69%, +2.04%]
  • ✔️graph_from_data_frame: 217ms -> 215ms [-3.21%, +1.05%]
  • ✔️vs_attr_filter: 234ms -> 234ms [-0.71%, +0.44%]
  • ✔️vs_by_name: 211ms -> 213ms [-1.07%, +2.82%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 840234c is merged into claude/readme-rmd:

  • ✔️as_adjacency_matrix: 178ms -> 178ms [-0.57%, +1.14%]
  • ✔️as_biadjacency_matrix: 199ms -> 199ms [-0.66%, +0.88%]
  • ✔️as_data_frame_both: 200ms -> 201ms [-0.28%, +0.87%]
  • ✔️as_long_data_frame: 162ms -> 160ms [-2.9%, +1%]
  • ✔️es_attr_filter: 189ms -> 191ms [-0.64%, +2.65%]
  • ✔️graph_from_adjacency_matrix: 215ms -> 218ms [-0.36%, +2.76%]
  • ✔️graph_from_data_frame: 216ms -> 216ms [-0.62%, +0.89%]
  • ✔️vs_attr_filter: 230ms -> 230ms [-0.15%, +0.82%]
  • ✔️vs_by_name: 211ms -> 211ms [-0.67%, +0.46%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
krlmlr added this pull request to stack #2907 September 13, 2026 21:25
@krlmlr
krlmlr force-pushed the claude/roxygen2-fork branch from 840234c to 2a68aa0 Compare September 13, 2026 21:26
commonmark discards the whitespace a line break stands for, so roxygen prose written
one sentence per line loses the gap between sentences in the rendered help.
Only the text renderer is affected, which is what `?topic` shows.

A new composite action installs roxygen2 from upstream with the R/ part of
krlmlr/roxygen2@f-sentence-spacing applied on top, and runs just before the
Roxygenize step. It shallow-clones upstream, fetches the branch, applies the diff
restricted to R/ so conflicts in the test files cannot fail it, and aborts rather
than silently installing an unpatched build. It then asserts that what it installed
really carries the patch.

Config/roxygen2/version becomes 8.1.0.9100. The .9100 suffix distinguishes a patched
build from upstream's own .9000 development builds; if upstream moves, the x.y.z part
follows it and the suffix stays. DESCRIPTION is DCF and cannot carry a comment, so the
explanation lives in a Config/cynkra/roxygen2 field.

This is a separate decision from the line-break reformatting below it, and is kept in
its own pull request so it can be taken or left on its own. Without it, the
reformatting simply renders as it does today, with one space between sentences.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WWhverMTZZKgEpUuTK117m
@krlmlr
krlmlr force-pushed the claude/roxygen2-fork branch from 2a68aa0 to cb1d208 Compare September 13, 2026 21:41
@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if cb1d208 is merged into claude/readme-rmd:

  • ✔️as_adjacency_matrix: 189ms -> 190ms [-1.75%, +2.82%]
  • ✔️as_biadjacency_matrix: 197ms -> 198ms [-3.63%, +4.04%]
  • ✔️as_data_frame_both: 175ms -> 174ms [-2.33%, +0.9%]
  • ✔️as_long_data_frame: 141ms -> 144ms [-0.91%, +4.47%]
  • ✔️es_attr_filter: 187ms -> 185ms [-3.48%, +2%]
  • ✔️graph_from_adjacency_matrix: 226ms -> 223ms [-3.71%, +1.14%]
  • ✔️graph_from_data_frame: 193ms -> 193ms [-2.04%, +1.94%]
  • ✔️vs_attr_filter: 189ms -> 192ms [-0.52%, +3.15%]
  • ✔️vs_by_name: 168ms -> 167ms [-2.63%, +1.95%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

claude and others added 2 commits September 13, 2026 22:08
…l escape

The version stamp wrote the replacement as "\1". R parses that as the octal
escape for \001, not as a regex backreference, so DESCRIPTION ended up with a
malformed version and R CMD INSTALL aborted with "Malformed package version".
Every job that installs roxygen2 through this action failed there. The
replacement is now "\\1", verified to stamp 8.1.0.9000 to 8.1.0.9100.

The post-install guard asserted only the .9100 suffix, and the corrupt
"\001.9100" satisfies that too, which is why the bug survived the check meant
to catch it. The guard now asserts the whole x.y.z.9100 shape, and passes
inherits = FALSE to exists().

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WWhverMTZZKgEpUuTK117m
@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if c3533b7 is merged into claude/readme-rmd:

  • ✔️as_adjacency_matrix: 246ms -> 245ms [-0.77%, +0.39%]
  • ✔️as_biadjacency_matrix: 262ms -> 262ms [-0.35%, +0.87%]
  • ✔️as_data_frame_both: 264ms -> 265ms [-0.25%, +1.15%]
  • ✔️as_long_data_frame: 211ms -> 210ms [-1.38%, +0.41%]
  • ✔️es_attr_filter: 248ms -> 248ms [-0.73%, +0.36%]
  • ✔️graph_from_adjacency_matrix: 275ms -> 275ms [-0.33%, +0.57%]
  • ✔️graph_from_data_frame: 278ms -> 277ms [-0.58%, +0.49%]
  • ✔️vs_attr_filter: 336ms -> 337ms [-0.36%, +0.74%]
  • ✔️vs_by_name: 319ms -> 318ms [-0.55%, +0.28%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Sign up for free to 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.

2 participants