klmr / box

Write reusable, composable and modular R code
https://klmr.me/box/
MIT License
829 stars 47 forks source link

Allow tidyselect-like syntax within `use()` #319

Open wurli opened 1 year ago

wurli commented 1 year ago

This could be seen as an extension of #287.

E.g. use(purrr[contains("map")]) would import map(), imap(), map_chr(), map_dbl() etc. Another common pattern might be use(ggplot2[ggplot, aes, starts_with("geom_")]). In my opinion this would do a lot for readability/convenience, and would lend familiarity for those who already use the tidyverse.

The only possible issue I can see is that tidyselect would use everything() rather than ... to import the whole namespace. I suppose supporting both might be possible, but perhaps not very neat.

klmr commented 1 year ago

Do you have a more concrete use-case in mind where this would be worthwhile? At the moment I am having a hard time justifying the complexity this would add (both to the code base and to the documentation). In particular, the examples you listed can already be expressed by either importing all desired names explicitly (recommended) or by using the wildcard import ... (generally not recommended). And while I can kind of see the “convenience” argument, I am not convinced that this improves readability.

Fundamentally, using these functions has all the disadvantages of a wildcard import, because it attaches implicit names. The main issue is that this can silently break user code when a module adds new exports that now override other imports.

I’m also comparing with module systems in other languages, and as far as I can see none of them offers an equivalent of what you’re proposing — indicating that this isn’t an important feature.

wurli commented 1 year ago

I think your arguments against this are solid, so if you decide it's not worth adding, fair enough. I think my main reason for the suggestion is this is that I don't want to have to reuse lengthy use() statements at the tops of all my modules. If I want all map() functions I'd rather use(purrr[contains("map")]) then have to manually include all 62 variations. I agree it's less robust, but I think it's clearer in some sense - e.g. it's not easy to confirm you haven't forgotten any map() variations with the current system.

Having said that, the more I think about it, the more I think this feature is at odds with the core purpose of {box}.