Skip to content

docs: use lifecycle instead of zzz-deprecate.R#716

Closed
maelle wants to merge 38 commits intoigraph:mainfrom
maelle:zzz-automated-deprecation
Closed

docs: use lifecycle instead of zzz-deprecate.R#716
maelle wants to merge 38 commits intoigraph:mainfrom
maelle:zzz-automated-deprecation

Conversation

@maelle
Copy link
Copy Markdown
Contributor

@maelle maelle commented Mar 16, 2023

Fix #697

⚠️ The work is done by inst/deprecate.R and inst/deprecate-template.txt

@krlmlr the code in inst/deprecate.R takes a while to run because it re-parses the whole package at every iteration, to have the most recent line numbers, since each iteration adds lines of code. 🤪

Current "preview" maelle#3

TODO

Comment thread inst/deprecate.R Outdated
@krlmlr

This comment was marked as resolved.

Comment thread inst/deprecate.R Outdated
@maelle

This comment was marked as resolved.

@maelle

This comment was marked as outdated.

@krlmlr

This comment was marked as resolved.

@maelle

This comment was marked as resolved.

@maelle maelle force-pushed the zzz-automated-deprecation branch from 89d59e9 to 620d1f4 Compare March 16, 2023 16:08
@codecov

This comment was marked as outdated.

@maelle

This comment was marked as resolved.

@ntamas
Copy link
Copy Markdown
Member

ntamas commented Mar 18, 2023

In this particular case, edge_disjoint_paths(), adhesion() and edge_connectivity() are different names for the same graph concept: how many edge-disjoint paths there are between a given pair of vertices (or, if the pair is not specified, between any pair of vertices in the input graph)? edge_disjoint_paths() is a bit of a misnomer because I would expect such a function to return the paths themselves and not only the number of paths.

Personally, if I were to design the API today from scratch, I'd only keep edge_connectivity() (and maybe adhesion() as an alias - this name is not used very commonly in graph theory but I believe it's used by social scientists, who frequently have their own names for common graph theory concepts). edge_disjoint_paths() may be used some time later in the future to implement a function that returns the paths themselves, but we don't have such a function in the C core yet.

(To add insult to injury, the C core has separate functions: igraph_edge_disjoint_paths() returns the number of edge disjoint paths between a given pair of vertices, while igraph_edge_connectivity() returns it for all source-target pairs. The R function decides to call one or another based on whether source and target are given or not).


Edit: I believe the same applies to cohesion(), vertex_connectivity() and vertex_disjoint_paths().

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Mar 20, 2023

@ntamas in zzz-deprecate.R there is

#' @export edge.disjoint.paths
deprecated("edge.disjoint.paths", edge_disjoint_paths)

should this be changed? should the deprecation message refer to edge_disjoint_paths() or, instead, edge_connectivity()?

So in general, if in zzz-deprecate.R something was deprecated to a function A, that is an alias of function B, do we want the documentation and deprecation message to refer to function A or function B? It's an easy switch in my script.

@ntamas
Copy link
Copy Markdown
Member

ntamas commented Mar 20, 2023

I think that this deprecation should point to edge_connectivity() (so, function B).

@maelle maelle force-pushed the zzz-automated-deprecation branch from 652dfde to 5afa6ea Compare March 27, 2023 06:29
@maelle

This comment was marked as outdated.

@maelle

This comment was marked as outdated.

This was referenced Mar 28, 2023
@maelle maelle force-pushed the zzz-automated-deprecation branch from 1d556d9 to 6509427 Compare May 15, 2023 08:53
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented May 15, 2023

interesting bug: I can't run this code if I run load_all() first, as it prevents pkgdown:::get_rd_from_help("igraph", fn_name) from working 🤯

@krlmlr

This comment was marked as outdated.

@maelle

This comment was marked as outdated.

@maelle maelle mentioned this pull request May 30, 2023
@krlmlr

This comment was marked as outdated.

@aviator-app

This comment was marked as outdated.

@maelle

This comment was marked as outdated.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Nov 13, 2023

Should be unblocked once the PRs are merged.

@aviator-app aviator-app Bot force-pushed the zzz-automated-deprecation branch from d55b522 to b20a224 Compare November 18, 2023 10:25
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

I will start a fresh PR from main then run the script after

  • running R CMD check on this branch to catch errors
  • changing the version in the lifecycle text to 1.6.0.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

"Rd files with duplicated alias" all over the place.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

apart from the alias situation, tests will need to be updated.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

a function is deprecated to make_famous_graph() that is not exported. #715

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

Note: possible error in 'shapes(shape = shape, ': unused arguments (clip = clip, plot = plot, parameters = parameters)
Note: possible error in 'triangles(graph = graph, ': unused argument (vids = vids)
Note: possible error in 'sample_pref(nodes = nodes, ': unused argument (type.dist.matrix = type.dist.matrix)
Note: possible error in 'curve_multiple(graph = graph, ': unused argument (x = x)
Note: possible error in 'distance_table(graph = graph, ': unused arguments (weights = weights, unconnected = unconnected, details = details)
Note: possible error in 'cohesive_blocks(blocks = blocks, ': unused argument (blocks = blocks)
Note: possible error in 'sample_last_cit(n = n, ': unused arguments (types = types, attr = attr)
Note: possible error in 'membership(communities = communities, ': unused arguments (no = no, steps = steps)
Note: possible error in 'betweenness(graph = graph, ': unused argument (e = e)
Note: possible error in 'sample_traits_callaway(nodes = nodes, ': unused argument (k = k)
Note: possible error in 'cohesive_blocks(blocks = blocks, ': unused arguments (blocks = blocks, file = file, project.file = project.file)
Note: possible error in 'as_adj_list(graph = graph, ': unused argument (.x = .x)
Note: possible error in 'distance_table(graph = graph, ': unused arguments (from = from, to = to, mode = mode, weights = weights)
Note: possible error in 'distance_table(graph = graph, ': unused arguments (from = from, to = to, mode = mode, weights = weights, output = output, predecessors = predecessors, inbound.edges = inbound.edges, algorithm = algorithm, x = x, x = x)
Note: possible error in 'graph_from_adj_list(adjlist = adjlist, ': unused argument (x = x)
Note: possible error in 'is_bipartite(types = types, ': unused arguments (types = types, edges = edges, directed = directed)
Note: possible error in 'as_data_frame(d = d, ': unused arguments (d = d, directed = directed, vertices = vertices)
Note: possible error in 'connect(graph = graph, ': unused arguments (nodes = nodes, mindist = mindist)
Note: possible error in 'graphlet_basis(graph = graph, ': unused arguments (cliques = cliques, niter = niter, Mu = Mu)
Note: possible error in 'graph_from_graphnel(graphNEL = graphNEL, ': unused arguments (n = n, l = l)
Note: possible error in 'shapes(coords = coords, ': unused arguments (coords = coords, el = el, params = params, end = end)
Note: possible error in 'shapes(coords = coords, ': unused arguments (coords = coords, v = v, params = params)
Note: possible error in 'as_graphnel(graph = graph, ': unused arguments (x = x, x = x, y = y)
Note: possible error in 'subgraph(graph = graph, ': unused argument (impl = impl)
Note: possible error in 'cohesive_blocks(blocks = blocks)': unused argument (blocks = blocks)
Note: possible error in 'cliques(graph = graph, ': unused arguments (subset = subset, file = file, x = x)
Note: possible error in 'cliques(graph = graph, ': unused argument (subset = subset)
Note: possible error in 'is_matching(graph = graph, ': unused arguments (weights = weights, eps = eps)
Note: possible error in 'distance_table(graph = graph, ': unused arguments (v = v, to = to, mode = mode, weights = weights, algorithm = algorithm)

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

this is wrong

average.path.length <- function(graph , weights = NULL , directed = TRUE , unconnected = TRUE , details = FALSE) { # nocov start
   lifecycle::deprecate_soft("1.6.0", "average.path.length()", "distance_table()")
   distance_table(graph = graph, weights = weights, directed = directed, unconnected = unconnected, details = details)
} # nocov end

deprecated_df is ok though.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

probably a regex I wrote wrong 👀

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

aha the problem is in how I use aliases to find arguments

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

in shapes.Rd, aliases are used for things that are not aliases. 🤔 not in the igraph codebase, but in how pkgdown handles things, so I need to update my assumptions and the code without breaking #716 (comment) again!

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 20, 2023

For things such as edge_connectivity() I'll update zzz-deprecate.R manually, and I'll stop using aliases as they don't make any sense for this use case.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 27, 2023

@krlmlr can you please explain b20a224? I'll reproduce it in my other PR.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Nov 27, 2023

The methods that this commit removes are never called because the "dendPlot" generic doesn't exist. The igraph::dendPlot() function has UseMethod("plot_dendrogram"), this is what is used for dispatch.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Nov 27, 2023

Please help me understand why you think the ivs (maxcohesion?) things should come first. Can't we add the deprecation later, manually? Eventually, we'll need to be able to do it manually anyway.

Yes, ok.

@maelle maelle closed this Nov 27, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use {lifecycle} instead of zzz-deprecate.R

3 participants