fix: report a deprecated function passed as an argument, and un-deprecate the layout callbacks - #2844
Closed
krlmlr wants to merge 5 commits into
Closed
fix: report a deprecated function passed as an argument, and un-deprecate the layout callbacks#2844krlmlr wants to merge 5 commits into
krlmlr wants to merge 5 commits into
Conversation
igraph functions that take another function as an argument call it on the user's behalf. lifecycle attributes a deprecation to whoever called the deprecated function, and that caller is igraph, not the user: `deprecate_soft()` stays silent, and `deprecate_warn()` blames igraph and asks the user to report a bug against it. Either way `plot(g, layout = layout.circle)` never tells the user that the layout function they passed is deprecated, until the deprecation becomes hard and the call fails outright. `check_deprecated_function()` reads the deprecation off the function object and replays it against the environment igraph was called from, with the same signaller and the same message, so lifecycle recognizes the deprecation the function goes on to signal as the one already reported and does not repeat it. Every argument that takes a function now goes through the check: plotting parameters, layouts, vertex shapes, attribute combinations, scan statistics, printer callbacks and the search callbacks. `layout.spring()`, `layout.svd()` and `layout.fruchterman.reingold.grid()` are deprecated with a warning again, and lay out with `layout_with_fr()` as before. They are layout callbacks, so their hard deprecation -- made after 2.3.3 and never released -- would break `plot(layout = )` for users who were never warned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW
…lback-functions-87kfjp
Contributor
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if b415cbe is merged into main:
|
The tests ran in an environment that belongs to igraph, and lifecycle exempts the test suite of the deprecating package from its "is this the user's doing?" question. Both made the tests pass whether or not the check reported anything. Call from an environment that belongs to no package, with the exemption cleared, and assert that the deprecation does not carry lifecycle's "likely used in the igraph package, please report the issue" footer -- which is what a user sees today, and the point of the exercise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW
…ions-87kfjp' into claude/deprecated-callback-functions-87kfjp
1 task
Contributor
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e5cec1c is merged into main:
|
Contributor
Author
|
Closing in favour of #2845, which attributes the deprecation by calling the function from the caller's environment rather than by inspecting it. Generated by Claude Code |
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.
This PR was prepared with Claude Code (Claude Opus).
#2845 is an alternative to this PR, closing the same gap by changing where the callback is called instead of inspecting it. Only one of the two should land; that PR's description compares them.
The problem
igraph functions that take another function as an argument call it on the user's behalf. lifecycle attributes a deprecation to whoever called the deprecated function, and that caller is igraph, not the user. So
plot(g, layout = layout.circle)was silent, whilelayout.circle(g)warned:deprecate_soft()stays quiet entirely, because the call does not come from the global environment;deprecate_warn()does warn, but appends "The deprecated feature was likely used in the igraph package. Please report the issue at …", pointing the user at our issue tracker for something they wrote themselves.Either way the user is not told that the function they passed is on its way out, until the deprecation becomes hard and the call fails outright.
setup-lifecycle.Ralready flags this class of deprecation in our own test suite (#2782); this closes the same gap for users.The check
check_deprecated_function()reads the deprecation off the function object — thelifecycle::deprecate_*()call among the statements of its body whosewhatnames the function itself — and replays it against the environment igraph was called from, using the same signaller and the same message.Replaying verbatim is what keeps this from doubling up: lifecycle deduplicates by message, so the deprecation the function goes on to signal when igraph calls it is recognized as the one already reported, and stays silent. Replaying with the original signaller also preserves the deprecation level, so a soft deprecation still only reaches the user who caused it, and a defunct function still errors.
A deprecation nested deeper in the body is conditional on the arguments of the call — it deprecates an argument or one of its values, like
bfs(father = ), not the function — and is left to the call itself.Every argument that takes a function now goes through the check:
plot(),tkplot(),rglplot(), thelayoutgraph attribute and function-valuedigraph_options();layout_nicely(),layout_components()andtk_reshape();clipandplotfunctions ofadd_shape();simplify(),contract(),union()and friends;local_scan(FUN = )and through itscan_stat();bfs(),dfs(),cliques(),max_cliques(),motifs(),simple_cycles(),isomorphisms(),subgraph_isomorphisms()andcluster_leading_eigen().Un-deprecating the layout callbacks
layout.spring(),layout.svd()andlayout.fruchterman.reingold.grid()were made defunct in #2634. That change is onmainonly — neither 2.3.2 nor 2.3.3 shipped it — and these are exactly the functions users hand toplot(layout = ), where the deprecation warning has never reached them. Making them error before they have ever warned in that position breaks working code without notice.They now warn again and lay out with
layout_with_fr(), as in 2.3.3. Their documentation already described that behaviour. The three NEWS bullets announcing the hard deprecation are dropped, since they are in an unreleased section and no longer describe what happens.Not touched, for the record:
layout.grid.3d()was removed in the same PR, but it was already defunct in 2.1.0 and in every released 2.3.x, so no working code depended on it. It is worth deciding separately whether a removed callback should keep a defunct stub for the sake of the error message.Testing
New
tests/testthat/test-utils-deprecated.Rcovers the detection, the replay at each level, the deduplication, the argument sites above, and that the three layout functions work again.Getting the tests to mean anything took some care: they run in an environment that belongs to igraph, and lifecycle exempts the test suite of the deprecating package from its "is this the user's doing?" question, so a naive test passes with or without the change. They call through an
as_user()helper — an environment that belongs to no package, withTESTTHAT_PKGcleared — and assert both that the deprecation is reported and that it does not carry lifecycle's "likely used in the igraph package, please report the issue" footer.Full suite passes; the two failures seen locally are environmental (a blocked download in
test-foreign.R, and acallrsubprocess intest-other.Rthat needs an installed igraph).🤖 Generated with Claude Code
https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW