Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vec_depth fails if list has values of class "name" #818

Closed
yogat3ch opened this issue Feb 15, 2021 · 4 comments · Fixed by #901
Closed

vec_depth fails if list has values of class "name" #818

yogat3ch opened this issue Feb 15, 2021 · 4 comments · Fixed by #901
Labels
bug an unexpected problem or unintended behavior pluck 🍐

Comments

@yogat3ch
Copy link

yogat3ch commented Feb 15, 2021

Hi Team purrr,
Grateful for the work ya'll have done on this package. I've used purrr - 974 times since I first became aware of it. Needless to say, it's an essential part of my R toolkit - so thank you!

I'm actually trying to come up with a solution for Advanced R 18.5.3 Q4 and have just used rapply to convert an expression to a series of lists with:
.calls <- rapply(as.list(x), as.list, classes = "call", how = "list")
I would like to map over the resulting list using purrr with something like the following:
purrr::map_depth(.calls, purrr::vec_depth(.calls), call_match_lgl, pattern = "fn")

But I'm finding that an element of class name apparently causes vec_depth to error.

Here's a simple reprex to illustrate:

purrr::vec_depth(as.list(rlang::call2(mean, 1:10, na.rm = TRUE)))

It seems like modifying it like this might make it return results match with what one would expect when visualizing with lobstr::ast:

vec_depth <- function(x) {
  if (is_null(x)) {
    0L
  } else if (is_atomic(x)) {
    1L
  } else if (rlang::is_symbol(x) || rlang::is_function(x)) {
    1L
  } else if (is_list(x) || rlang::is_call(x)) {
    depths <- map_int(as.list(x), vec_depth)
    1L + max(depths, 0L)
  } else {
    abort("`x` must be a vector")
  }
}

There might be some exceptions where this would have unexpected results though.
Thoughts?

@hadley
Copy link
Member

hadley commented Aug 24, 2022

Somewhat more minimal reprex:

purrr::vec_depth(list(x = quote(x)))
#> Error in `.f()` at purrr/R/map.R:204:2:
#> ! `x` must be a vector

Created on 2022-08-24 by the reprex package (v2.0.1)

It feels like the final else should just return 0. I don't think this function needs to handle all the other types of object that you might index into with [ or [[. (So the documentation should also be updated)

@hadley hadley closed this as completed Aug 24, 2022
@hadley hadley reopened this Aug 24, 2022
@hadley hadley added bug an unexpected problem or unintended behavior list 🧦 labels Aug 24, 2022
@hadley
Copy link
Member

hadley commented Aug 27, 2022

Maybe this should be pluck_depth()?

@yogat3ch
Copy link
Author

Maybe this should be pluck_depth()?

If making a function to do the task as described in the example, that would certainly make sense as a name!

hadley added a commit that referenced this issue Aug 28, 2022
hadley added a commit that referenced this issue Aug 29, 2022
And generalise to handle more types of input.

Fixes #818
@yogat3ch
Copy link
Author

Checked out the changes and they look solid! Thanks @hadley & @lionel- 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior pluck 🍐
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants