Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make recursion optional #150

Closed
wants to merge 5 commits into from
Closed

Conversation

courtiol
Copy link

@courtiol courtiol commented Sep 11, 2022

Fixes #149

The idea of this PR is the reduce a little further the clutter while remaining accurate.
Here is an example with dplyr, a package with a lot of dependencies.
The basic plot is quite unreadable because it shows too much:

library(miniCRAN)
plot(makeDepGraph(pkg = "dplyr"))

The situation improves if suggested packages are turned off:

plot(makeDepGraph(pkg = "dplyr", suggests = FALSE))

But one could argue, do we really need here to show e.g. pkgconfig which is only on indirect dependencies (imported by tibble, which is itself imported by dplyr).

What I did was thus to refine the internal call to tools::package_dependencies() so as to exploit the argument it contains called recursive. It was hard-coded as TRUE but now the user can control it:

plot(makeDepGraph(pkg = "dplyr", suggests = FALSE, recursive = FALSE))

Note that contrary to the original argument recursive in tools::package_dependencies() which allows for both logical or character vector input, I restricted the use to a logical only. I explored the alternative but it would lead to quite a lot of changes in the code.

I paid attention to update the package examples, tests, news and description.

R CMD check does not complain about anything on my system.

@courtiol courtiol marked this pull request as draft September 11, 2022 15:22
@courtiol
Copy link
Author

There are still glitches I am trying to solve...

@courtiol courtiol marked this pull request as ready for review September 11, 2022 16:00
@andrie
Copy link
Owner

andrie commented Mar 24, 2024

I suspect the better way to do this is to compute on the dependency graph itself, rather than to limit recursion.

In igraph you can compute the shortest distance to a node: https://r.igraph.org/reference/distances.html?q=distance#null

It should then be possible to remove all nodes with a distance greater than n.

@courtiol
Copy link
Author

Thanks @andrie, would you like to implement that?
It has been 2 years I submitted this so I forgot the structure of your package in the meantime and I don't know anything about igraph...

@andrie andrie closed this Mar 24, 2024
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.

Controling arg recursive passed to tools::package_dependencies()
2 participants