Skip to content

refactor: move optional arguments of conversion functions behind the ellipsis - #2766

Merged
krlmlr merged 4 commits into
mainfrom
claude/ellipsis-conversion
Jul 27, 2026
Merged

refactor: move optional arguments of conversion functions behind the ellipsis#2766
krlmlr merged 4 commits into
mainfrom
claude/ellipsis-conversion

Conversation

@krlmlr

@krlmlr krlmlr commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Part of the repo-wide ellipsis migration coordinated in #2757
see that PR for the full rationale (CONTRIBUTING.md, Argument Order and the Ellipsis).
Based directly on main (the registry split landed via #2779); a single topic commit.

What this does

Inserts ... between the defining arguments (head) and the optional
modifiers
(tail) of 11 exported conversion functions.
Arguments after ... become keyword-only.

  • Legacy positional or abbreviated calls are recovered by the generated
    ARG_HANDLE block and emit a single lifecycle::deprecate_soft("3.0.0", …)
    behavior is unchanged.
  • No defaults change, no arguments are renamed, deprecated functions untouched.
  • Registry: tools/migrations/conversion.R; blocks regenerated via
    Rscript tools/generate-migrations.R (idempotent, CI-checked).
  • Rd usage/arguments updated mechanically (see note below).

New signatures

function head (positional) keyword-only tail
from_adjacency() adjmatrix mode, weighted, diag, add.colnames, add.rownames
graph_from_adjacency_matrix() adjmatrix mode, weighted, diag, add.colnames, add.rownames
as_adj_edge_list() graph mode, loops
as_adj_list() graph mode, loops, multiple
as_directed() graph mode
as_edgelist() graph names
graph_from_adj_list() adjlist mode, duplicate
graph_from_data_frame() d, directed vertices
graph_from_edgelist() el directed
graph_from_graphnel() graphNEL name, weight, unlist.attrs
graph_from_biadjacency_matrix() incidence directed, mode, multiple, weighted, add.names

Notes for review

  • The environment used to prepare this PR cannot install igraph.r2cdocs
    (GitHub API unreachable), so man/*.Rd files were updated mechanically
    (usage + ... argument item) instead of via devtools::document().
    R CMD check's usage↔formals validation passes; a follow-up
    devtools::document() run may reflow whitespace but should produce no
    semantic diff.
  • Package tests that called these functions positionally were updated to
    named arguments (they are the same soft-deprecation user code will see).

@krlmlr
krlmlr force-pushed the claude/ellipsis-conversion branch 2 times, most recently from f439827 to b2fa62d Compare July 26, 2026 11:09
@github-actions

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 768ms -> 766ms [-1.17%, +0.52%]
  • ✔️as_biadjacency_matrix: 737ms -> 741ms [-0.38%, +1.26%]
  • ✔️as_data_frame_both: 1.68ms -> 1.69ms [-1.27%, +2.21%]
  • ❗🐌as_long_data_frame: 4.22ms -> 4.34ms [+1.33%, +4.37%]
  • ✔️es_attr_filter: 3.11ms -> 3.12ms [-2.18%, +2.81%]
  • ✔️graph_from_adjacency_matrix: 117ms -> 117ms [-0.61%, +2.01%]
  • ✔️graph_from_data_frame: 3.59ms -> 3.57ms [-1.64%, +0.95%]
  • ✔️vs_attr_filter: 1.72ms -> 1.74ms [-0.27%, +3.58%]
  • ✔️vs_by_name: 1.11ms -> 1.12ms [-1.31%, +2.53%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
krlmlr marked this pull request as ready for review July 26, 2026 15:03
@krlmlr krlmlr closed this Jul 26, 2026
@krlmlr
krlmlr force-pushed the claude/ellipsis-conversion branch from b2fa62d to 0f7d122 Compare July 26, 2026 16:24
…ellipsis

Insert `...` between the defining arguments and the optional
modifiers of 11 functions,
following the zoning rules in CONTRIBUTING.md.
Legacy positional and abbreviated calls are recovered
by the generated ARG_HANDLE blocks
(registry: tools/migrations/conversion.R)
and emit a single soft deprecation for igraph 3.0.0.
No defaults change and no arguments are renamed.

Functions: as_adj_edge_list, as_adj_list, as_directed, as_edgelist, from_adjacency, graph_from_adj_list, graph_from_adjacency_matrix, graph_from_biadjacency_matrix, graph_from_data_frame, graph_from_edgelist, graph_from_graphnel

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
@krlmlr krlmlr reopened this Jul 26, 2026
Same fix as #2781: the migration rewriter collapsed multi-line enum
defaults and air skips c() calls, so restore the original layout.

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

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 825ms -> 824ms [-1.31%, +1.05%]
  • ✔️as_biadjacency_matrix: 792ms -> 794ms [-0.87%, +1.6%]
  • ✔️as_data_frame_both: 1.62ms -> 1.64ms [-1.3%, +3.5%]
  • ✔️as_long_data_frame: 4.07ms -> 4.16ms [-0.11%, +4.48%]
  • ✔️es_attr_filter: 2.88ms -> 2.93ms [-2.15%, +5.82%]
  • ✔️graph_from_adjacency_matrix: 128ms -> 129ms [-0.63%, +2.02%]
  • ✔️graph_from_data_frame: 3.53ms -> 3.57ms [-0.39%, +2.74%]
  • ✔️vs_attr_filter: 1.67ms -> 1.65ms [-3.86%, +2.09%]
  • ✔️vs_by_name: 1.06ms -> 1.06ms [-2.23%, +2.47%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

claude added 2 commits July 26, 2026 19:58
Add argument-coverage tests for the 11 conversion-topic functions
moved to keyword-only tails:
every tail argument is passed by name with a non-default value
where the file did not already exercise it,
and each function's legacy positional call
is checked to warn and recover.

The graph_from_graphnel() tests reuse the existing
skip_if_not_installed("graph") guard,
so they are exercised on CI only.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
The two internal callers passed `mode` positionally,
which reaches the recovery path of the migrated signatures
and would warn in the graphNEL tests on CI
(lifecycle treats the package under test as a direct caller).

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

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 880ms -> 881ms [-1.71%, +1.95%]
  • ✔️as_biadjacency_matrix: 823ms -> 817ms [-3.11%, +1.6%]
  • ❗🐌as_data_frame_both: 1.74ms -> 1.78ms [+0.65%, +4.14%]
  • ✔️as_long_data_frame: 4.44ms -> 4.53ms [-2.55%, +6.74%]
  • ✔️es_attr_filter: 2.8ms -> 2.79ms [-3.05%, +1.9%]
  • ✔️graph_from_adjacency_matrix: 129ms -> 127ms [-3.4%, +0.84%]
  • ✔️graph_from_data_frame: 3.78ms -> 3.77ms [-1.76%, +1.52%]
  • ✔️vs_attr_filter: 1.63ms -> 1.63ms [-3.12%, +3.07%]
  • ✔️vs_by_name: 1.01ms -> 1.01ms [-4.17%, +3.21%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
krlmlr merged commit 769c011 into main Jul 27, 2026
9 checks passed
@krlmlr
krlmlr deleted the claude/ellipsis-conversion branch July 27, 2026 17:56
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