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

Make modify_depth() and map_depth() consistent with modify_tree() #960

Merged
merged 13 commits into from
Sep 27, 2022

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 23, 2022

Fixes #958

@hadley hadley requested a review from lionel- September 23, 2022 16:30
NEWS.md Outdated Show resolved Hide resolved
@@ -8,7 +8,8 @@
#' @param leaf A function applied to each leaf.
#' @param is_leaf A predicate function that determines whether an element is
#' a leaf (by returning `FALSE`) or a node (by returning `FALSE`). The
#' default value, `NULL`, treats lists as nodes and everything else as leaves.
#' default value, `NULL`, treats bare lists as nodes and everything else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to vec_is_list()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave this as is since once I switch to is_node instead of is_leaf the default will become vec_is_list() and then it's obvious.

R/modify-tree.R Outdated Show resolved Hide resolved

x <- list(data.frame(x = 1), data.frame(y = 1))
expect_equal(
map_depth(x, 2, class, .is_leaf = Negate(is.list)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of Negate makes me wonder if it wouldn't be better to take is_node instead of is_leaf?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wondered about this too. I might do it in a separate PR.

tests/testthat/test-modify-tree.R Outdated Show resolved Hide resolved
hadley and others added 10 commits September 27, 2022 17:01
Co-authored-by: Lionel Henry <lionel.hry@gmail.com>
* Use `.error_call` not `.call`

* Bump required dev vctrs version
Create new vctrs compatibility helper, and use it in a few places. "Weird" vectors like pairlists, expressions, and calls have been deprecated.
@hadley hadley merged commit 07c00ac into main Sep 27, 2022
@hadley hadley deleted the node-spec branch September 27, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is_leaf argument for modify_depth()
3 participants