Refactor draw_legend()#463
Conversation
grantmcdermott
left a comment
grantmcdermott
left a comment
There was a problem hiding this comment.
I'm a little nervous to split the legend functionality into a serious of nested functions, given how hard tricky it can be to debug and troubleshoot already. But these simple parameter assignments look fine and you do have a track record of effective modularization so I'm willing to take it on faith ;-)
Minor nits remaining.
| } | ||
|
|
||
|
|
||
| swap_variables <- function(env, ...) { |
There was a problem hiding this comment.
Nit: = instead of '<-` please.
(I should add a CLAUDE.md to catch these nits at some point...)
| pairs <- list(...) | ||
| for (p in pairs) { | ||
| tmp <- get(p[1], envir = env) | ||
| assign(p[1], get(p[2], envir = env), envir = env) | ||
| assign(p[2], tmp, envir = env) | ||
| } | ||
| } | ||
|
|
||
| swap_columns <- function(dp, a, b) { | ||
| va <- dp[[a]] | ||
| vb <- dp[[b]] | ||
| dp[[a]] <- if (!is.null(vb)) vb else NULL | ||
| dp[[b]] <- if (!is.null(va)) va else NULL | ||
| dp |
| if (!is.null(log)) log <- chartr("xy", "yx", log) | ||
| datapoints <- swap_columns(datapoints, "x", "y") | ||
| datapoints <- swap_columns(datapoints, "xmin", "ymin") | ||
| datapoints <- swap_columns(datapoints, "xmax", "ymax") |
Oh yeah, totally. Honestly, I don't think I would have gotten as good of an appreciation for your point before trying to make that PR work. The execution order with My hope/expectation is that changes like this actually make things easier to navigate, because it'll be easier for us to see when/where there are side effects.
Should be done. |
|
Out atm but go for it if you're happy and tests are passing. |
2 participants
The purpose of this PR is to simplify the logic of
draw_legends()somewhat. The main strategy is to have a cleaner separate of concerns, with a cleaner difference between functions that are called to compute coordinates/settings and functions that are called to create side-effects.I want to do this in a few more ways and think it will open up big long term benefits, perhaps in the same vein as the
type_*()refactor.