Skip to content

docs: fix dots docs thanks to roxygen2's warning about useless inheritParams - #2868

Open
maelle wants to merge 5 commits into
mainfrom
roxy2
Open

docs: fix dots docs thanks to roxygen2's warning about useless inheritParams#2868
maelle wants to merge 5 commits into
mainfrom
roxy2

Conversation

@maelle

@maelle maelle commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Co-Authored-By: Claude Sonnet 5

Running document() generated warnings about useless inheritParams.

I think that when we added dots to force users to name optional parameters, we might have not paid enough attention to the resulting manual pages. When a function with the "rlang dots" was documented on the same page as a function with other dots (dots actually forwarded to another function), the documentation used the docs of that other dots params, which is incorrect.

@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 81e2dd3 is merged into main:

  • ✔️as_adjacency_matrix: 812ms -> 804ms [-2.27%, +0.5%]
  • ✔️as_biadjacency_matrix: 795ms -> 787ms [-2.23%, +0.39%]
  • ✔️as_data_frame_both: 1.83ms -> 1.8ms [-3.52%, +0.98%]
  • ✔️as_long_data_frame: 4.58ms -> 4.44ms [-6.09%, +0.02%]
  • ✔️es_attr_filter: 2.81ms -> 2.81ms [-2.73%, +2.56%]
  • ✔️graph_from_adjacency_matrix: 124ms -> 125ms [-0.4%, +2.84%]
  • ✔️graph_from_data_frame: 3.98ms -> 3.95ms [-2.37%, +0.69%]
  • ✔️vs_attr_filter: 1.61ms -> 1.61ms [-2.13%, +2.98%]
  • ✔️vs_by_name: 1.05ms -> 1.02ms [-6.16%, +1.04%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Comment thread man/sir.Rd
\item{x}{A \code{sir} object, returned by the \code{sir()} function.}

\item{\dots}{Additional arguments, ignored currently.}
\item{\dots}{For \code{sir()} and \code{time_bins()}, these dots must be empty. For

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether in the cases of having the dots for S3 method consistency, the code should error and then we could stop documenting the dots differently.

@maelle
maelle marked this pull request as ready for review August 28, 2026 10:01
@maelle
maelle requested a review from schochastics August 28, 2026 10:15
@krlmlr

krlmlr commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Thanks. Let's do another review.

How to avoid this happening again? When we are at zero warnings, we could set strict roxygen2 mode without warnings in CI/CD.

@schochastics

Copy link
Copy Markdown
Contributor

There is no built-in strict mode to flip: on roxygen2 8.0.0.9000 the supported Roxygen: fields (per roxygen2:::load_options()) are roclets, packages, load, old_usage, markdown, r6, rd_family_title, knitr_chunk_options, restrict_image_formats, lazy_data. So it has to be "fail CI on any roxygen2 warning", i.e. a change to .github/workflows/roxygenize/action.yml, which today is a bare roxygen2::roxygenize():

warns <- character()
withCallingHandlers(
  roxygen2::roxygenize(),
  warning = function(w) {
    warns <<- c(warns, conditionMessage(w))
    invokeRestart("muffleWarning")
  }
)
if (length(warns)) {
  stop(
    "roxygen2 emitted ", length(warns), " warning(s):\n",
    paste("-", warns, collapse = "\n")
  )
}

I would prefer that over options(warn = 2), so that unrelated warnings from loading the package don't blow up the job.

Two caveats before turning it on:

  • That action runs dev roxygen2, whose parsing is deliberately stricter than 7.x, so a roxygen2 bump can turn unrelated PRs red. Pinning the version, or running the gate as a separate job so the reason is obvious, would help.
  • We were not at zero yet: cliques documents ... manually in the parent block, so the five @inheritParams rlang::args_dots_empty in the same topic still produced "@inheritParams failed. All parameters are already documented; none remain to be inherited." I removed those five (the weighted_cliques parent keeps its own, it is the only dots doc on that topic).

Also pushed in the same batch, from re-reading the diff against the actual signatures:

  • merge_coords(): dropping the @inheritParams there left the shared text as "Additional arguments to pass to the layout layout function", which is only true for layout_components(); merge_coords() errors on any dots. Split like the others.
  • The two new topics: layout_spec is titled for add_layout_() only, although layout_(g, with_fr()) is the main entry point, and constructor_spec covers 4 of the 46 constructor_spec() wrappers while also being used by graph_(). Retitled both, added examples, and moved them to their own families (layout specifications / constructor specifications) with matching _pkgdown.yml entries, so they no longer show up in the "deterministic constructors" list (degseq() is not deterministic) and don't add an opaque as_bipartite() / from_data_frame() link to every layout and constructor page.

One thing I did not touch: the tkplot dots text omits tk_reshape(), whose dots are forwarded to the layout function. Since tkplot is deprecated and unmaintained, I left it alone.

@maelle

maelle commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Danke @schochastics!

schochastics and others added 3 commits September 3, 2026 13:43
…pics

- cliques: drop five `@inheritParams rlang::args_dots_empty` that roxygen2
  rejects because the parent block already documents `...` for the topic.
  The `weighted_cliques` parent keeps its own, it is the only dots doc there.
- merge_coords(): its dots must be empty, only `layout_components()` forwards
  them to the layout function. Split the shared text accordingly.
- layout_spec: retitle for `layout_()` as well as `add_layout_()`, which is
  the main entry point, and add examples.
- constructor_spec: retitle to include `graph_()`, point at the other
  specification functions documented with their constructors, and add
  examples.
- Give both topics their own family instead of "deterministic constructors"
  (`degseq()` is not deterministic) and "graph layouts", with matching
  `_pkgdown.yml` entries.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if c4c4584 is merged into main:

  • ✔️as_adjacency_matrix: 807ms -> 811ms [-1.49%, +2.62%]
  • ✔️as_biadjacency_matrix: 818ms -> 811ms [-2.69%, +1.04%]
  • ✔️as_data_frame_both: 1.71ms -> 1.72ms [-1.64%, +3.44%]
  • ✔️as_long_data_frame: 4.11ms -> 4.15ms [-0.49%, +2.21%]
  • ✔️es_attr_filter: 2.86ms -> 2.87ms [-2.98%, +3.31%]
  • ✔️graph_from_adjacency_matrix: 150ms -> 149ms [-2.46%, +0.99%]
  • ✔️graph_from_data_frame: 3.85ms -> 3.85ms [-1.91%, +1.87%]
  • ✔️vs_attr_filter: 1.66ms -> 1.68ms [-3.37%, +6%]
  • ✔️vs_by_name: 1.09ms -> 1.06ms [-9.46%, +2.97%]
    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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants