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

Better approach for mocking base functions #1853

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Better approach for mocking base functions #1853

merged 2 commits into from
Sep 18, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 15, 2023

It just occurred to me that you only need a binding, not a specific implementation. So this makes mocking base functions a little bit easier.

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

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

I don't have a strong opinion here.

But this did remind me of some unpleasantness when usethis had an internal helper that masked a base R function, i.e. this whole saga:

rstudio/rstudio#8031

I also think this would mean that, after load_all(), a call to said base function would also not work for interactive development-time tinkering or in the debugger.

None of these concerns seem like a huge deal. But the potential for creating puzzles or obstacles for yourself makes me think that, if you need to mock something, it's probably best to just wrap it and keep everything nice and obvious.

@hadley
Copy link
Member Author

hadley commented Sep 15, 2023

I don't think it will interfere with your use of said functions because interative() will ignore non-function objects in the search path. So it might only be confusing if you type interactive.

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.

Makes sense!

The other thing we could do, when we detect the user wants to mock a base function, is to insert an unlocked environment between imports and base? This way users wouldn't need to think about the difference between imported functions (which are bound in an environment private to the package) and base functions (which are shared across all packages).

@hadley hadley merged commit bccc249 into main Sep 18, 2023
12 checks passed
@hadley hadley deleted the mocking-base branch September 18, 2023 18:18
@hadley
Copy link
Member Author

hadley commented Sep 18, 2023

@lionel- yeah, that might be worth considering, although I'm not sure if modifying parent.env() is much better than (e.g.) calling rlang::env_unlock() on the package env.

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.

3 participants