search

baptiste / gridExtra

Miscellaneous Functions for "Grid" Graphics
http://cran.r-project.org/web/packages/gridExtra/index.html
15 stars 4 forks source link

Backwards compatibility broken #18

Closed RemkoDuursma closed 8 years ago

RemkoDuursma commented 9 years ago

Dear developers. We just noticed that recent changes in gridExtra have broken backwards compatibility in ellipseGrob. It used to have an argument ar (aspect ratio), which has been changed to rho (the inverse of ar).

Apart from scratching my head why this change was made (is there some advantage?), it would be ideal if ar was still an argument so as to not break lots of code.

dfalster commented 9 years ago

+1

baptiste commented 9 years ago

how much code is broken? Would it not be easier to make a little wrapper for older code, and update it progressively?

grid.ellipse <- function(ar, ...) { gridExtra::grid.ellipse(rho=1/ar, ...) }

I emailed >50 package maintainers depending on gridExtra before this update, and no-one seemed to use the function. In fact, I didn't think of including it until I realised I needed it myself in a project. I had to clean up the implementation, which wasn't very clear.

RemkoDuursma commented 9 years ago

It is hard to tell how much code is broken - the package maintainers that explicitly depend on gridExtra form a small component of the actual users, I would imagine. My question is more on why compatibility was broken in this case - I see no advantage to the new argument rho instead of ar. I fully understand the tradeoffs when maintaining a package, and sometimes it needs to be broken so it can be fixed.

I don't see how a wrapper is easier than changing the call to ellipseGrob directly in old code?

Why not have an argument ar=NULL and then if(!is.null(ar))rho <- 1/ar ? This is fairly common in packages that have changed the default arguments.

baptiste commented 9 years ago

i don't have a strong view against changing rho to ar, but it may not reach CRAN very soon. See if the development version works for you.

RemkoDuursma commented 9 years ago

Thanks - I will test it as soon as I can.