Skip to content

adding .onAttach for smooth data loading #16

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benyamindsmith
Copy link

When igraphdata is presently loaded. The data does not look nice when the library is called without igraph. I added a zzz.R file and an .onAttach function.

Without .onAttach

Screenshot 2024-11-14 200949

With .onAttach

This is a fix:

image

Let me know if this is helpful or not.

@benyamindsmith
Copy link
Author

image

I will update and resubmit.

@benyamindsmith
Copy link
Author

The issue appears to be with macos. Not sure what needs to be done here.

krlmlr added a commit that referenced this pull request Nov 24, 2024
krlmlr added a commit that referenced this pull request Nov 24, 2024
krlmlr added a commit that referenced this pull request Nov 25, 2024
krlmlr added a commit that referenced this pull request Nov 26, 2024
krlmlr added a commit that referenced this pull request Nov 27, 2024
krlmlr added a commit that referenced this pull request Dec 6, 2024
krlmlr added a commit that referenced this pull request Dec 6, 2024
* ci: Use stable pak

* ci: Correctly detect branch protection

* ci: Use Ubuntu 24.04 and styler PR

* ci: Use Ubuntu 24.04 for fledge, squash

* ci: Fix macOS (#16)

* ci: Need to install R on Ubuntu 24.04

* ci: Need to install R on Ubuntu 24.04 for fledge

* ci: Use styler from main branch

* ci: Explicit permissions

* ci: Ignore errors when removing pkg-config on macOS

* ci: Use larger retry count for lock-threads workflow
@benyamindsmith
Copy link
Author

@krlmlr - a gentle reminder about this :)

Curious if this is going to be merged or not.

@maelle
Copy link
Contributor

maelle commented Mar 10, 2025

Hello @benyamindsmith! Nice idea. Do you think users often load igraphdata without loading igraph first?

In any case this reminds me of #16

@maelle
Copy link
Contributor

maelle commented Mar 10, 2025

Curious to hear what @schochastics @krlmlr think

@schochastics
Copy link
Contributor

I think it is save to assume that if you use igraphdata, you will also have igraph loaded. However, I still think this is reasonable to merge since there might be use cases I do not see atm

@benyamindsmith
Copy link
Author

@maelle

I personally have been doing development/scripting in R with libname::funcname sort of syntax. So when I was running code before with igraph I often did not write library(igraph) on the top of my scripts and would just use igraph::funcname.

This is what motivated my PR.

Beyond my personal reason, I can't see why one would leave this out as using .onAttach ensures that the datasets are presented as intended.

@szhorvat
Copy link
Member

I do not feel qualified to state an opinion here since I am an R novice.

But out of curiosity I tried to find other packages that do this and I could not. I looked here:

I do wonder why.

Is it not a convenience during interactive work to have the autoload? I often end up loading igraphdata without loading igraph first, just out of forgetfulness. But it isn't really an issue as it seems perfectly fine to load igraph after loading the data and still use the datasets.

@schochastics
Copy link
Contributor

There are other packages that do this, just not with igraph (at least my quick search turned out empty). You can check on the github repo of CRAN:
https://github.com/search?q=org%3Acran%20%22.onAttach%20%3C-%20function(libname%2C%20pkgname)%22%20%20%22requireName%22&type=code

@krlmlr
Copy link
Contributor

krlmlr commented Mar 13, 2025

Thanks. I now see what the problem is -- igraph is imported but the import isn't actually used when igraphdata is loaded. One way to make sure loading happens is to add

#' @importFrom igraph V
NULL

or similar. This adds an entry to NAMESPACE which in turn ensures that igraph is loaded as soon as igraphdata is loaded. I suspect there is little need for using the igraphdata package without igraph.

@benyamindsmith
Copy link
Author

@krlmlr could we get a sanity check on that to confirm that the @importFrom specification makes .onAttach irrelevant? A screenshot would be good enough.

@krlmlr
Copy link
Contributor

krlmlr commented Mar 26, 2025

.onAttach() is weaker than the namespace declaration. With the latter, the following code will work as expected in a fresh R session:

igraphdata::karate

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.

5 participants