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

Move data.table to Imports #31

Open
MichaelChirico opened this issue Dec 11, 2024 · 6 comments · May be fixed by #32
Open

Move data.table to Imports #31

MichaelChirico opened this issue Dec 11, 2024 · 6 comments · May be fixed by #32
Assignees
Labels
bug Something isn't working

Comments

@MichaelChirico
Copy link

Contact Details

No response

What happened?

Thanks for using data.table!

I'm filing because we strongly discourage using {data.table} as Depends (in fact I strongly discourage Depends in general besides of course for R). This is something we track: Rdatatable/data.table#3076. See also the vignette on importing data.table: https://cran.r-project.org/web/packages/data.table/vignettes/datatable-importing.html.

If you'd like any help migrating, please feel free to reach out. The most common hang-up in my experience is (possibly) needing explicit library(data.table) in tests/vignettes. Blanket import(data.table) in NAMESPACE is the easiest fix for the package code itself, though I do encourage selectively importing only what's needed.

Version

0.1.0 (Default)

Relevant log output

No response

@GregJohnsonJr
Copy link
Collaborator

Thank you for letting us know! The hyperlink was helpful in understanding the issue at hand. I will link the PR once we address this issue and we will keep you updated.

On a side note, and just for my understanding, this is recommended so users are not forced to install data.table correct?

@GregJohnsonJr GregJohnsonJr added the bug Something isn't working label Dec 11, 2024
@MichaelChirico
Copy link
Author

not quite! they will still need to install data.table.

Basically Depends means library(mpactr) will also run library(data.table) on the user's behalf. There are a wide variety of reasons why this is unappealing. One example is here:

library(lubridate)
library(mpactr)

there are a number of functions in lubridate that share a name with functions exported by data.table (I think hour, week, ...). As of now, library(mpactr) will cause those lubridate functions to be "hidden" behind the data.table ones.

The more packages out there using Depends, the gnarlier this problem becomes as it becomes quite hard to reason about the code just from reading it.

If the user wants data.table functions, they should just run library(data.table) -- this sort of transparency is very important for code maintenance & readability.

@GregJohnsonJr
Copy link
Collaborator

GregJohnsonJr commented Dec 11, 2024

Ahh, thank you so much for explaining that! If I were to compare that to a language like cpp, it seems like a namespace issue. I imagine its the same reason we use std::* or "mynamespace::"* to reference objects in different namespaces, or environments in this instance. I didn't realize how harmful the depends functionality was.

@MichaelChirico
Copy link
Author

Yes, AFAICT it's a relic of a time when R lacked the ability to have cleaner separation of package vs. user environments.

R still is not designed to handle a huge number of library imports -- as library() always attaches a full namespace vs. pythons explicitly recommending against 'from module import *' or similar rules about minimizing 'using lib;' to get blanket access to another library in C++.

@GregJohnsonJr
Copy link
Collaborator

Sorry for the delay, but thanks for all the helpful information! We are working to push the fix out this week. I will link a pull request to confirm that everything is in-order before I merge and send it to cran.

@MichaelChirico
Copy link
Author

Thanks! Don't feel the need to rush. It's more a long-term strategy to encourage using Imports, and to evangelize about the problems with Depends

@GregJohnsonJr GregJohnsonJr linked a pull request Dec 20, 2024 that will close this issue
@GregJohnsonJr GregJohnsonJr linked a pull request Dec 20, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants