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

Bind in imports #1748

Merged
merged 8 commits into from Mar 22, 2023
Merged

Bind in imports #1748

merged 8 commits into from Mar 22, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented Mar 4, 2023

Allow new mocking functions to also mock functions that are imported from other packages.

Since the package environment is locked, we can only change bindings there, not add them. But that's ok because if we're trying to mock something that's not in the package, it must be imported from another namespace.
@hadley hadley requested a review from lionel- March 21, 2023 13:07
Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

Looks great!

This could be extracted in a function, and then we could loop over the imports of all dependencies of the current package. Then we have full mocking across the whole session. This will be useful for packages that depend on each other in an integrated stack.

R/mock2.R Outdated
@@ -48,6 +63,18 @@ with_mocked_bindings <- function(code, ..., .package = NULL) {

# helpers -----------------------------------------------------------------

# Wrapper around local_bindings() that automatically unlocks and takes
# list of bindings.
local_env_bind <- function(env, bindings, frame = caller_env()) {
Copy link
Member

Choose a reason for hiding this comment

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

local_bindings_unlock()?

Copy link
Member

Choose a reason for hiding this comment

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

And take ... for consistency?

R/mock2.R Outdated
bindings_found[in_pkg] <- TRUE

# Bindings in imports
for (env in env_parents(ns_env)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice use of env_parents(), I remember when you suggested it should stop at globalenv() years ago :)

@hadley
Copy link
Member Author

hadley commented Mar 22, 2023

What are you thinking would be extracted into a function?

@lionel-
Copy link
Member

lionel- commented Mar 22, 2023

What are you thinking would be extracted into a function?

The imports updating with local cleanup, so you could apply it to the namespaces of all dependencies. Though I guess it has to be a bit more complicated because a same name could be bound to another function in other namespaces, so you'd need to check that it's the same function before mocking.

@hadley
Copy link
Member Author

hadley commented Mar 22, 2023

If I understand what you're saying, I think you can already do that with (e.g.)

local_mocked_bindings(sym = function(...) "x", .package = "rlang")

@lionel-
Copy link
Member

lionel- commented Mar 22, 2023

oh right. Well that does look good enough and also maybe safer to do it explicitly.

@hadley
Copy link
Member Author

hadley commented Mar 22, 2023

Yeah, for now, I wanted to make this explicit, because it somehow feels wrong to make it easy to change code owned by others.

@hadley hadley merged commit 3cb0f41 into main Mar 22, 2023
12 checks passed
@hadley hadley deleted the bind-in-imports branch March 22, 2023 15:33
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.

None yet

2 participants