wilkelab / cowplot

cowplot: Streamlined Plot Theme and Plot Annotations for ggplot2
https://wilkelab.org/cowplot/
704 stars 84 forks source link

Allow using plot_grid in a pipe #146

Closed flying-sheep closed 5 years ago

flying-sheep commented 5 years ago

Currently, one needs to do

foo %>% map(ggplot) %>% plot_grid(plotlist = .)

With this change, the following is possible:

foo %>% map(ggplot) %>% plot_grid()
codecov-io commented 5 years ago

Codecov Report

Merging #146 into master will decrease coverage by 1.1%. The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
- Coverage   68.69%   67.58%   -1.11%     
==========================================
  Files          20       20              
  Lines         856      799      -57     
==========================================
- Hits          588      540      -48     
+ Misses        268      259       -9
Impacted Files Coverage Δ
R/plot_grid.R 89.28% <50%> (-1.46%) :arrow_down:
R/as_grob.R 30.3% <0%> (-1.13%) :arrow_down:
R/key_glyph.R 95.38% <0%> (-0.77%) :arrow_down:
R/themes.R 96.21% <0%> (-0.3%) :arrow_down:
R/draw.R 46.83% <0%> (-0.23%) :arrow_down:
R/set_null_device.R 0% <0%> (ø) :arrow_up:
R/stamp.R 0% <0%> (ø) :arrow_up:
R/align_plots.R 100% <0%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 597b3ea...759fea9. Read the comment docs.

clauswilke commented 5 years ago

Since plot_grid() is meant to take most anything as an argument, this will lead to problems down the road. This is true regardless of the specific logic you employ to try to detect whether plot_grid() is employed inside a pipe.

flying-sheep commented 5 years ago

I think you misunderstood: The logic in my PR checks if the first and only argument only has the class 'list'. It therefore it breaks nothing, but instead makes the following work:

> # current behavior:
> plot_grid(list(ggdraw()))
Warning message in as_grob.default(plot):
“Cannot convert object of class list into a grob.”
> # behavior after my PR
> plot_grid(list(ggdraw()))
<an empty canvas!>
clauswilke commented 5 years ago

No, I didn't misunderstand. Rewriting function signatures based on the data type of some of the input is fundamentally a bad idea. In your case, if at some point in the future somebody finds a use for as_grob.list(), then that will unexpectedly not work with plot_grid(). And likewise, if somebody gets used to having plot_grid() behave a certain way with input of type list, they may expect the same to happen with input of type gList, and that's also not going to be possible.

flying-sheep commented 5 years ago

Why is it a bad idea? I don’t see how as_grob.list might in any way make sense.