briandk / granovaGG

Bob Pruzek and Jim Helmreich's implementation of Elemental Graphics for Analysis of Variance
Other
15 stars 4 forks source link

A recent upgrade to `gridExtra` broke the package #166

Closed briandk closed 7 years ago

briandk commented 8 years ago

A few days ago granovaGG started failing CRAN package checks, and a CRAN maintainer notified me.

Baptiste Auguie on 2016-02-28 wrote:

Most of the warnings appear due to a conflict between plyr::join and gridExtra::join. Packages listing gridExtra in Imports: should probably be selective in what they import (with importFrom directives in the NAMESPACE file), and that would solve the problem. For those few packages that list gridExtra in Depends: I'm not sure what to suggest other than moving either plyr or gridExtra to Imports: and be selective (this advice has been a recurring theme lately, to alleviate such name masking).

Since gridExtra::join is not a particularly apt name in the first place, I'll also consider renaming it, but selective imports are still useful regardless. (emphasis mine)

briandk commented 8 years ago

So I tweeted: https://twitter.com/Capbri/status/704345883102617600

briandk commented 8 years ago

In short, Baptiste is recommending an @importFrom solution while @hadley advises against it:

2016-02-29 - using importfrom in r packages 02-29-16 11 55 58 am

Source: https://cran.r-project.org/web/packages/roxygen2/vignettes/namespace.html

jennybc commented 8 years ago

Sounds like this is a good excuse to move towards, e.g., gridExtra::join(), instead of importing the entire namespace of gridExtra and calling join(). I don't find any uses of join() in this repo, so the conflict/masking must be coming from some other function. But that's really neither here nor there -- the pkg::foo() approach is a general practice people are adopting to eliminate these headaches. So it might also apply to reshape2 and plyr here, in addition to gridExtra.

briandk commented 8 years ago

@jennybc Thank you so much! This is wonderful advice. I think I've found a problematic edge case though, where I don't know what to do:

What happens when you only want to reference one function from a package, but that function references another function from a package. It seems like using double-colon notation doesn't work. In my case, I think I'm having a problem where gridExtar::grid.arrange() internally calls gridExtra::arrangeGrob(), and I don't know how to make sure that internal call works.

Here's an abstracted minimal example.

A minimal example of where foo::bar() notation gets tricky

Suppose your package depends on package foo, and you only use one function: foo::bar(). But, also suppose that the definition of foo::bar() depends on another function baz() in that package: foo::baz().

So what you do is

the definition of foo::bar() looks like this:

# @export
bar <- function(x) {
  return(baz(x))

}

foo's DESCRIPTION collate file looks like this:

Collate:
    'baz.R',
    'bar.R'

The Million Dollar Question

When your code calls foo::bar(), will it succeed?

/cc @WilDoane

WilDoane commented 8 years ago

It should work. Consider the (somewhat messy) case:

gg1 <- ggplot2::ggplot(mtcars) + ggplot2::geom_histogram(ggplot2::aes(x = hp))
gg2 <- ggplot2::ggplot(mtcars) + ggplot2::geom_histogram(ggplot2::aes(x = mpg))

gridExtra::grid.arrange(gg1, gg2)

when ggplot2 and gridExtra packages are not attached. it works fine and plots the result.

> devtools::session_info()
Session info -------------------------------------------------------------------------------------------------------
 setting  value                       
 version  R version 3.2.3 (2015-12-10)
 system   x86_64, mingw32             
 ui       RStudio (0.99.1081)         
 language (EN)                        
 collate  English_United States.1252  
 tz       America/New_York            
 date     2016-03-08                  

Packages -----------------------------------------------------------------------------------------------------------
 package       * version date       source        
 assertthat      0.1     2013-12-06 CRAN (R 3.2.2)
 colorspace      1.2-6   2015-03-11 CRAN (R 3.2.2)
 DBI             0.3.1   2014-09-24 CRAN (R 3.2.2)
 devtools        1.10.0  2016-01-23 CRAN (R 3.2.3)
 digest          0.6.9   2016-01-08 CRAN (R 3.2.3)
 dplyr           0.4.3   2015-09-01 CRAN (R 3.2.2)
 ggplot2         2.0.0   2015-12-18 CRAN (R 3.2.3)
 gridExtra       2.2.0   2016-02-27 CRAN (R 3.2.3)
 gtable          0.2.0   2016-02-26 CRAN (R 3.2.3)
 labeling        0.3     2014-08-23 CRAN (R 3.2.2)
 magrittr        1.5     2014-11-22 CRAN (R 3.2.2)
 memoise         1.0.0   2016-01-29 CRAN (R 3.2.3)
 munsell         0.4.3   2016-02-13 CRAN (R 3.2.3)
 plyr            1.8.3   2015-06-12 CRAN (R 3.2.2)
 R6              2.1.2   2016-01-26 CRAN (R 3.2.3)
 Rcpp          * 0.12.3  2016-01-10 CRAN (R 3.2.3)
 RevoUtilsMath * 3.2.3   2016-02-09 local         
 scales          0.3.0   2015-08-25 CRAN (R 3.2.3)
 tidyr           0.4.1   2016-02-05 CRAN (R 3.2.3)
> 
jennybc commented 8 years ago

As @WilDoane says, it will work. Do you feel like you're seeing something else? I mean, almost all functions in a package will call other exported and unexpected functions in that package, so this is not an uncommon situation.

To learn perhaps more than you wanted to know about this, I recommend:

http://blog.obeautifulcode.com/R/How-R-Searches-And-Finds-Stuff/