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

Add use_data_table #897

Closed
MichaelChirico opened this issue Sep 23, 2019 · 6 comments · Fixed by #924
Closed

Add use_data_table #897

MichaelChirico opened this issue Sep 23, 2019 · 6 comments · Fixed by #924

Comments

@MichaelChirico
Copy link
Contributor

Redirecting from here:

r-lib/roxygen2#461 (comment)

Goal is to offer a template to facilitate importing data.table analogously to existing use_tidy_eval, primarily relating to key data.table NSE symbols

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Sep 23, 2019

Am having some second thoughts here.

Two sources of hesitation:

  1. Leaning towards just doing @import data.table instead of plucking key objects with @importFrom (in which case use_data_table would be pretty trivial -- only one essential line). Any strong reason for preferring one over the other? It seems the former makes it more straightforward to do dev things without any visible difference to ultimate package users? What's the motivation behind using @importFrom rlang in the use_tidy_eval case?

  2. We're on a bit of a crusade to move people from Depending on data.table to Importing it. It would be great if use_data_table could help nudge users along here, but at least for use_tidy_eval DESCRIPTION is untouched --> still tempting to put us as a Depends. Perhaps this is something to add as a ui_todo? Any thoughts?

@jennybc
Copy link
Member

jennybc commented Sep 23, 2019

What's the motivation behind using @importFrom rlang in the use_tidy_eval() case?

I think we encourage selective importing (so individual @importFroms) in general over importing a whole namespace (@import) out of a sense of minimalism and explicitness. But I and others certainly import a whole namespace if the usage is extensive enough and rlang is a package that often gets handled that way. But not if it's only being used to support a tidy eval interface.

Any strong reason for preferring one over the other?

As for data.table, my knowledge of typical usage inside a package is too thin to say. So I think those of you who know will have to make the @import vs. @importFrom call. use_data_table() is really a chance to show people what you want the vast majority of developer's to be doing.: "here, do it like this."

Any thoughts? re: Depends vs Imports

You could certainly inspect the current DESCRIPTION for Depends: data.table and issue some messaging around that. use_data_table() will, in general, add it to Imports. I suppose you could even ask people for permission to move data.table from Depends to Imports, if you discover it there.

@MichaelChirico
Copy link
Contributor Author

Thanks Jenny. Quite helpful! Will ruminate a bit more on how we want to do it but I'm certainly convinced use_data_table() will be useful in some form or another :)

@hadley
Copy link
Member

hadley commented Oct 18, 2019

To close the loop (and maybe this issue 😉) is this something that you're still thinking about?

@hadley
Copy link
Member

hadley commented Oct 18, 2019

BTW on the import(data.table) issue, my current opinion is that's ok as long as the package you're importing is specifically design with that usage in mind, i.e. the api surface is designed to minimise potential future conflicts with other packages. I suspect data.table falls into that camp, so I think it would be fine to recommend. (My only caution would be that if people see you using it for a popular package like data.table, they might think it's ok for general usage)

@MichaelChirico
Copy link
Contributor Author

Thanks for the bump @hadley, have filed #924

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 a pull request may close this issue.

3 participants