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

Minor ?env_unbind issues: inherit example, undocumented behaviors #1736

Open
brookslogan opened this issue Jul 30, 2024 · 0 comments
Open

Minor ?env_unbind issues: inherit example, undocumented behaviors #1736

brookslogan opened this issue Jul 30, 2024 · 0 comments

Comments

@brookslogan
Copy link

brookslogan commented Jul 30, 2024

?env_unbind seems to imply that inherit = TRUE will remove a binding from all ancestors:

# With inherit = TRUE, it removes bindings in parent environments
# as well:
parent <- env(empty_env(), foo = 1, bar = 2)
env <- env(parent, foo = "b")

env_unbind(env, "foo", inherit = TRUE)
env_has(env, c("foo", "bar"))
env_has(env, c("foo", "bar"), inherit = TRUE)

but this is not the case, with the last two lines evaluating to

env_has(env, c("foo", "bar"))
#>   foo   bar 
#> FALSE FALSE 
env_has(env, c("foo", "bar"), inherit = TRUE)
#>  foo  bar 
#> TRUE TRUE 

Perhaps this could become

# With inherit = TRUE, it can remove bindings in parent environments
# as well:
parent <- env(empty_env(), foo = 1, bar = 2)
env <- env(parent, foo = "b")

env_unbind(env, "foo", inherit = TRUE) # removes from `env`
env_unbind(env, "foo", inherit = TRUE) # removes from `parent`
env_has(env, c("foo", "bar"))
#>   foo   bar 
#> FALSE FALSE 
env_has(env, c("foo", "bar"), inherit = TRUE)
#>   foo   bar 
#> FALSE  TRUE 

Additionally, it would be nice if the documentation guaranteed [a certain warning/error behavior when attempting to remove a binding that doesn't exist. Currently, that behavior is no error or warning if no binding is found to remove. (I originally thought this matched mutate behavior and would be handy for implementing mutate-like verbs, but although mutate allows you to "remove" nonexistent columns, the error message when attempting to remove grouping columns (! `vars` missing from `data`: [...]) suggests that's not supposed to be the case. So name-checking logic still seems like something that would need to be handled downstream in this case.)]

@brookslogan brookslogan changed the title Minor ?env_unbind issues: inherit example, include nonexistent binding behavior Minor ?env_unbind issues: inherit example, undocumented behaviors Sep 3, 2024
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

No branches or pull requests

1 participant