Replacement for system.file. Fixes #179 #213

Merged
merged 4 commits into from Dec 14, 2012

Conversation

Projects
None yet
2 participants
@wch
Member

wch commented Dec 14, 2012

This fixes #179. The insert_shim function now adds a replacement version of system.file into a packages imports environment. (This actually works out to be a lot cleaner than inserting it into the namespace environment.)

It could probably use better documentation in load_all, but otherwise I think should be good to go.

+# This function is called with the name of the package being loaded, and returns
+# a replacement function for system.file.
+# @param pkg_name The name of the package loaded with load_all
+shim_system.file <- function(pkg_name) {

This comment has been minimized.

Show comment Hide comment
@hadley

hadley Dec 14, 2012

Owner

I was thinking more like shims <- new.env(); shims$system.file <- ... and then I think you might be able to reuse the namespace code to copy in

@hadley

hadley Dec 14, 2012

Owner

I was thinking more like shims <- new.env(); shims$system.file <- ... and then I think you might be able to reuse the namespace code to copy in

This comment has been minimized.

Show comment Hide comment
@wch

wch Dec 14, 2012

Member

Here's why it's not a good idea to add this function in a namespace that gets imported: That function still needs to have some record of the loaded package -- it needs to know the name of the package (which is why it's a closure). if the closure is put into a namespace -- call it foo::system.file -- then there can be only one copy of that function in the whole R session. So if you load_all multiple packages, then it will return modified results only for one of them. It could work if the function didn't have to be a closure, but I can't think of a good way to do that.

@wch

wch Dec 14, 2012

Member

Here's why it's not a good idea to add this function in a namespace that gets imported: That function still needs to have some record of the loaded package -- it needs to know the name of the package (which is why it's a closure). if the closure is put into a namespace -- call it foo::system.file -- then there can be only one copy of that function in the whole R session. So if you load_all multiple packages, then it will return modified results only for one of them. It could work if the function didn't have to be a closure, but I can't think of a good way to do that.

@hadley

This comment has been minimized.

Show comment Hide comment
@hadley

hadley Dec 14, 2012

Owner

Looks good - just need to add an entry to NEWS

Owner

hadley commented Dec 14, 2012

Looks good - just need to add an entry to NEWS

@wch

This comment has been minimized.

Show comment Hide comment
@wch

wch Dec 14, 2012

Member

Updated and rebased.

Member

wch commented Dec 14, 2012

Updated and rebased.

wch added a commit that referenced this pull request Dec 14, 2012

Merge pull request #213 from wch/shim
Replacement for system.file. Fixes #179

@wch wch merged commit be1017c into r-lib:master Dec 14, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment