support for hidden functions/objects

[hutch3232]

Say I have a script, functions.R:

.script_version <- "v1.0"

message("you have loaded version: ", .script_version)

foo_a <- function(){

}

When I run:

import::from(.from = "functions.R", .all = TRUE, .character_only = TRUE)

It finds the hidden variable .script_version, prints the message, but then somewhere along the way discards it. I would really like it if the hidden object stayed hidden in the .into environment.

you have loaded version: v1.0
> ls(envir = as.environment("imports"))
[1] "?"     "foo_a"
> ls(envir = as.environment("imports"), all.names = TRUE)
[1] "?"     "foo_a"
> .script_version
Error: object '.script_version' not found

I have tried to identify where it may be getting lost, and I think perhaps here: https://github.com/rticulate/import/blob/169346c2e8321c9c3bc0105394b6242c6d77266c/R/from.R#L205

From ?ls:

all.names a logical value. If TRUE, all object names are returned. If FALSE, names which begin with a . are omitted.

I do have a work-around, where I assign it to the environment I want within in the script, but it's a little messier.

Thanks!

[torfason]

Thanks for the detailed report, and for finding one place in code that might be related to it. It looks like import is not importing hidden objects. In fact, it looks from my very preliminary testing that it will not import hidden objects even if specified, as in:

import::from(functions.R, .script_version)

which for me results in an error (@hutch3232 , is it same for you)?

And it also does not seem to allow importing hidden functions (your report had a non-hidden function, and then a hidden character vector).

So, given that import does not support importing hidden objects, adding that would be a new feature. It would have costs, for example, one would need to document that the object names .all, .from and so on are not allowed. And new control parameters added in the future could break code – say if it became useful to add a .script_version parameter for some reason. So making this change would require a compelling argument, I think. Is the need compelling enough?

[hutch3232]

Thanks for the quick reply!

which for me results in an error (@hutch3232 , is it same for you)?

It is, I get this error:

Error in unlist(list(...)) : object '.script_version' not found

I made a test build of the package, making just one change to the line mentioned above and just threw in the extra argument:

all_objects <- ls(scripts[[from]], all.names = TRUE)

which actually is consistent with an earlier line: https://github.com/rticulate/import/blob/169346c2e8321c9c3bc0105394b6242c6d77266c/R/from.R#L166

which I think is why import recognizes my hidden object long enough to print my message, but then it is not preserved.

Anyways, I didn't do significant testing, but that change worked for me!

You do bring up good points about cost/benefit of this change. Personally I think this is somewhat of a common use-case, where function creators may have little helper functions that they don't want to be user-facing so they make them hidden. I also think users would expect hidden objects to be preserved when first using the package (esp. because they appear to be live momentarily). However, there are work-arounds with assign that people in this situation could use, if the costs are still too high.

Thanks for considering! Also happy to help however I can.

[torfason]

Thanks for looking more closely at this. I'm sure it would be beneficial in some cases. As it is, imports prefixed with a point (.function) are not deterministic and would not be recommended. Although the fix is small, the bigger issue is the implication that they are supported as first-class. For example, with this fix, users would expect this be legal (if version was defined in functions.R:

import::from(functions.R, .script_version=version)

But later on, a .script_version parameter might be added to the import packages (and it's not even such a far fetched thought, given that versioning is already supported for packages). Which would then break code that had been supported. So the caveats would need to be clearly documented and tests added to ensure that the behavior in case of conflicts was predictable.

Let's keep this open, I think we will at least give this serious thought when the next version comes up (no dates have been set for that).

[torfason]

On closer consideration, the fact that dot-prefixed imports might later be illegal if new dot-prefixed params were added is perhaps not enough of a reason to block this fix. It might be enough to include a warning in the documentation that any given such function might break in the future if a parameter were added.

So if this is desired by users, and someone volunteers to implement this (including some tests and docs) we could add this.

[hutch3232]

Hi @torfason, I am trying to make a PR addressing this issue. You should be able to see it in my fork of this repo: https://github.com/hutch3232/import/tree/hidden-objects. When I went to make the PR I only saw a master branch, rather than the dev branch referred to in #61. Some additional tests may be needed, but thought it was a decent starting point. FYI this is my first attempt at contributing to an open source project, so definitely open to feedback.

[torfason]

First, thanks for the contribution!

Because the staging area is in a different repo ( https://github.com/torfason/import ) the PR process is a bit more involved. I'm looking into it with @smbache, whether it makes sense to move it to a more traditional place (dev branch within rticulate).

But in the meantime, we won't let PR formalities get in our way. If any good code exists anywhere on a public branch, we can find a way to get it in. I just went ahead and pulled it in, and it is now available on

• https://github.com/torfason/import/tree/prx/hidden-objects

Builds and tests all run locally for me, and the GitHub actions all pass as well. Great job!

• Because this is a new feature, and one that must include a warning about possible naming clashes in the future, we'll need some new documentation as well. At a minimum the help doc needs to be modified, probably somewhere around here:

#' The function arguments can be quoted or unquoted as with e.g. \code{library}.
#' In any case, the character representation is used when unquoted arguments are
#' provided (and not the value of objects with matching names). The period in
#' the argument names \code{.into} and \code{.from} are there to avoid name
#' clash with package objects. The double-colon syntax \code{import::from}
#' allows for imports of exported objects (and lazy data) only. To import
#' objects that are not exported, use triple-colon syntax, e.g.
#' \code{import:::from}. The two ways of calling the \code{import} functions
#' analogue the \code{::} and \code{:::} operators themselves.

• Probably the vignette should be updated as well, and possibly (I'm not sure, you could take a look) the README.Rmd as well.

• We need to update NEWS.md with this new feature.

It would also be good to break the implementation up into three commits:

• Tests (all in a single commit, which is the first commit)
• The code implementation
• Documentation (including news)

Why tests as a first commit? I'm finding that it is very convenient to be able to check out a version with the test but without the fix, so that I can see exactly how they fail before the fix is implemented. It's a variation on TDD (test driven development)

Just let me know which of these tasks you are up for. You can add the code, and have me sort out the commit order, or you can restructure the commits yourself. If you work off of (https://github.com/torfason/import) there is less rebasing for me to do, but if you feel more comfortable just working on your current version, that is also fine.

Just let me know here when you'd like me to take another look. Or if you have any questions.

[hutch3232]

Thanks for all of these details. I think I can try to work on each of these items over the next few days. I see the new dev branch you made as well, for when it comes time for me to actually make a PR.

[torfason]

Fixed in dev via PR #64 (manually merged)

[torfason]

All features/fixes targeted for v1.3.0 are now in dev and master has been forwarded to match dev, and the version bumped to correspond to that. All checks required for a CRAN release see to be passing, and the plan is to submit to CRAN after the weekend, unless any unknown issues are uncovered until then. In the meantime, it would be much appreciated if the people involved in the new features/fixes could check out this could install and try out the most recent version from GitHub, for example using:

pak::pak("rticulate/import")

(remotes::install_github("rticulate/import") or devtools::install_github("rticulate/import") should give the same result)

Thanks to everyone for their contributions!

[torfason]

Fixed in v1.3.0