tinyplot.R cosmetic refactor#464
Conversation
|
FYI, I changed very little code in this PR, but I moved many lines around, which makes review difficult. Sorry! Here's the link to the whole file: https://github.com/grantmcdermott/tinyplot/blob/ef588b88eb3daebe730a284fa5e402437d8a3ffa/R/tinyplot.R |
grantmcdermott
left a comment
grantmcdermott
left a comment
There was a problem hiding this comment.
Did a quick review during my lunch break.
One nit(ish) meta comment that I didn't make: I'm not the biggest fan of block headings b/c they are slight pain to write and also don't index well in my IDE/dev environment for quick navigation between sections. Could we maybe use something that is more "breadcrumb" friendly like:
#
## some section -----
#?
|
|
||
| ########################### | ||
| # save parameters and calls | ||
| ########################### |
There was a problem hiding this comment.
Here and several other places: Can you please make sure there is a 1-line gap between the section header and the subsequent code.
| if (is.null(bg) && !is.null(fill)) bg = fill | ||
|
|
||
| # will be overwritten by some type_data() functions and ignored by others | ||
| # ribbon.alpha is overwritten by some type_data() functions |
There was a problem hiding this comment.
maybe prefix comment with # note: ...
| if (!null_by && is.character(by)) by = factor(by) | ||
| x_by = identical(x, by) # "boxplot", "spineplot" and "ridge" | ||
|
|
||
| # flag if x==by (currently only used for "boxplot", "spineplot" and "ridges" types) |
There was a problem hiding this comment.
Can we keep this old description? I know the code isn't difficult to read, but I feel explicit comments are better.
| tmp = sanitize_axes(axes, xaxt, yaxt, frame.plot) | ||
| list2env(tmp[c("axes", "xaxt", "yaxt", "frame.plot")], environment()) | ||
| rm("tmp") | ||
|
|
||
| # xlab & ylab | ||
| tmp = sanitize_xylab( | ||
| x = x, xlab = xlab, x_dep = x_dep, xmin_dep = xmin_dep, xmax_dep = xmax_dep, | ||
| y = y, ylab = ylab, y_dep = y_dep, ymin_dep = ymin_dep, ymax_dep = ymax_dep, | ||
| type = type) | ||
| xlab = tmp$xlab | ||
| ylab = tmp$ylab | ||
| rm("tmp") |
There was a problem hiding this comment.
Again, I have an aversion to tmp variables (even if cleaned up directly). Could we not just assign/subset directly, e.g.:
list2env(sanitize_axes(...)[<args_you_want_to_keep>])?
There was a problem hiding this comment.
Second comment on these sections, should we briefly say what each of these sanitize_* functions are meant to do? In the spirit of more explicit commenting, I think it would be helpful to state here what we expect them to return, so that we don't have to look up the the internal cross-ref. (I don't recall offhand whether they are internally documented...)
| rm("tmp") | ||
|
|
||
| # facet | ||
| facet_by = FALSE |
There was a problem hiding this comment.
similar to x_by, maybe a more explicit comment?
| bubble_alpha = if (!is.null(alpha)) alpha else 1 | ||
| bubble_bg_alpha = if (!is.null(bg) && length(bg)==1 && is.numeric(bg) && bg > 0 && bg <=1) bg else 1 | ||
| } | ||
|
|
| } | ||
|
|
||
| # plot limits | ||
| # after yaxb |
There was a problem hiding this comment.
New comment isn't clear to me. Are we saying we only want to calculate limits after x/yaxb have been calculated?
| xlim_user = xlim_user, ylim_user = ylim_user, | ||
| type = type | ||
| ) | ||
| fargs = fargs[c("xlim", "ylim")] |
There was a problem hiding this comment.
Should we subset directly in the line above?
There was a problem hiding this comment.
Actually, maybe we just assign/subset directly in the list2env call like I proposed above. fargs is a bad name for the temp variable anyway... i'm not sure why we (I?) called it that in the first place.
| # Determine the number and arrangement of facets. | ||
| # Note: We're do this up front, so we can make some adjustments to legend cex | ||
| # next (if there are facets). But the actual drawing of the facets will only | ||
| # come later. |
There was a problem hiding this comment.
Hmmm. I prefer this old, more explicit comment. Maybe we can add some more detail to the new terse on below (l. 992).
| # | ||
| ## Exterior plot elements (plot and facet windows, axes, etc.) | ||
| # |
There was a problem hiding this comment.
Can we re-use some version of this comment that makes the logic explicit? The point I was trying to convey, which was tricky to figure out originally, is that you first need to determine and draw all of the exterior elements (facets, axes, etc.) in your plot window, before you circle back to each facet and and draw the interior elements (grouped points, lines, etc.).
P.S. Agree that this is a good idea. One simple solution would be to roll our own variant that provides the option (or requires?) the explicit return list. |
|
Thanks for the review @grantmcdermott I agree with all your comments and have implemented all of them except for 1: avoid temporary objects and stricter The reason is that this PR is just the last cosmetic step toward implementing my Master Plan (tm). I'll open a new WIP / spec PR soon with a concrete proposal, and if you like that idea, it'll make all these messy calls and temp variables obsolete. If it doesn't work out, I can circle back to this and cleanup the residual So I'd just like to merge the cosmetics for now, and move on the actual substantive proposal. |
|
Sgtm. I'm excited to see the master plan! Feel free to merge if you're happy. |
2 participants
This PR does four things:
draw_title()helper function.list2env()calls more explicit. They are convenient and I was the one who introduced them, but I now feel that they are quite a bad idea, because they obscure which variables are being passed to (and potentially) overwritten by a function. This is a first step toward a future refactor to make internal argument passing easier and more robust.I consider this a self-contained contribution ready for review.