tidyomics / plyranges

A grammar of genomic data transformation
https://tidyomics.github.io/plyranges/
137 stars 19 forks source link

Should plyranges reexport tidyverse generics? #63

Closed sa-lee closed 5 years ago

sa-lee commented 5 years ago

There's been a lot of discussion about this, see this thread https://twitter.com/_lionelhenry/status/1106455607933054982.

An option would be to register the methods on load like the sf package does: https://github.com/r-spatial/sf/blob/master/R/tidyverse.R

@lawremi do you think this is important or not?

Tangentially related to #57

lawremi commented 5 years ago

Do you mean export or register? If we have methods, then we should register them in NAMESPACE. The only reason to do it dynamically is if the generics may not be loaded, but we already import dplyr, at least.

sa-lee commented 5 years ago

I mean reexport the generics. The methods are registered in the NAMESPACE already and we import dplyr. I'm having trouble understanding the reasoning not to reexport as mentioned in that thread.

lawremi commented 5 years ago

Typically we use Depends to automatically attach packages defining generics, so that the user gets them on the search path. It's just a convenience to the user. It seems messy to export generics defined by another package. I'd prefer that packages import each generic from the defining package.

It looks like plyranges does not Depend on dplyr. Is that to avoid symbol conflicts?

sa-lee commented 5 years ago

That was the original reason why I set it up that way, but now I'm not so sure that was the best decision...

sa-lee commented 5 years ago

One other option to not reexport could be explicit renaming of the dplyr generics to verb_ranges. Makes it explicit that the return type will always be a ranges object and avoids symbol conflicts.

lawremi commented 5 years ago

I think it's justified given the conflicts between the tidyverse and Bioconductor to just export the generics.

sa-lee commented 5 years ago

Okey thanks for your input :smile: