Use vec_is_list() in map_depth() #926
Conversation
R/map-if-at.R
Outdated
| } else { | ||
| if (vec_is_list(.x)) { | ||
| map(.x, map_depth_rec, .depth - 1, .f, ..., .ragged = .ragged) | ||
| } else { | ||
| if (.ragged) { | ||
| map(.x, .f, ...) | ||
| } else { | ||
| abort("List not deep enough") | ||
| } | ||
| } |
There was a problem hiding this comment.
Would the control flow be easier to read as a flat if-else chain?
There was a problem hiding this comment.
You mean within the depth > 1 block?
R/map-if-at.R
Outdated
| } else if (.depth == 1) { | ||
| map(.x, .f, ...) | ||
| } else { | ||
| if (vec_is_list(.x)) { |
There was a problem hiding this comment.
Probably we still want to recurse into data frames, since map() treats them as lists?
There was a problem hiding this comment.
You're the one who wanted vec_is_list() here 😆 But yeah, I think the key the switch from is.atomic() to !is.list(). (And probably include expressions for compatibility with pluck_depth()/pluck())
There was a problem hiding this comment.
I do want vec_is_list(), but purrr may require special-casing data frames.
There was a problem hiding this comment.
I think maybe we should stick with is.list() for now; it feels like we need to ensure that pluck(), pluck_depth(), and map_depth() should all be consistent.
Or maybe we should have purrr_is_list() which would be vec_is_list(x) || is.data.frame(x)? But that doesn't work for pluck() since it makes sense to (e.g.) pluck elements out of a linear model.
There was a problem hiding this comment.
yeah, that's probably safest. Ideally you'd either use accessors or unclass before plucking, with the understanding that you're reaching into unexported internals, but I guess it's too late to be this strict.
hmm... But did is_atomic() use to allow reaching into linear models with map_depth()?
There was a problem hiding this comment.
I think so, just because of the way we flipped testing for a leaf vs. testing for recursion.
|
Need to make a similar change to |
And document them together
* No usage on CRAN * Genericity in modify() should be sufficient
Conflicts: NEWS.md man/map_if.Rd
|
I've merged this since it's a big improvement and effectively orthogonal to what the recursion critera should be. |
| @@ -63,10 +63,6 @@ | |||
| * Some mapping functions have now a `.progress` argument to create a | |||
| progress bar. See `?progress_bars` (#149). | |||
|
|
|||
| * purrr is now licensed as MIT (#805). | |||
Fixes #920