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

Wrapper has the same interface as the wrapped function #14

Merged
merged 2 commits into from
Jan 19, 2016
Merged

Wrapper has the same interface as the wrapped function #14

merged 2 commits into from
Jan 19, 2016

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Sep 12, 2015

Also, the wrapped function is referred to by memoised_function, which is clearer than f.

Cc @maxheld83 and @piccolbo.

Fixes #12.

Fixes #6.

Closes #7 (=supersedes it).

Closes #2 (=includes it).

Closes #13 (=includes it).

Example:

> memoize(runif)
function (n, min = 0, max = 1) 
{
    hash <- digest(list(n, min, max))
    if (cache$has_key(hash)) {
        res <- cache$get(hash)
    }
    else {
        res <- withVisible(memoised_function(n = n, min = min, 
            max = max))
        cache$set(hash, res)
    }
    if (res$visible) {
        res$value
    }
    else {
        invisible(res$value)
    }
}
<environment: 0x3c32040>
attr(,"memoised")
[1] TRUE
> memoize(paste)
function (..., sep = " ", collapse = NULL) 
{
    hash <- digest(list(..., sep, collapse))
    if (cache$has_key(hash)) {
        res <- cache$get(hash)
    }
    else {
        res <- withVisible(memoised_function(... = ..., sep = sep, 
            collapse = collapse))
        cache$set(hash, res)
    }
    if (res$visible) {
        res$value
    }
    else {
        invisible(res$value)
    }
}
<environment: 0x3d687f0>
attr(,"memoised")
[1] TRUE

@krlmlr krlmlr changed the title WIP: Wrapper has the same interface and internally calls the wrapped function by its name Wrapper has the same interface and internally calls the wrapped function by its name Sep 12, 2015
@krlmlr krlmlr changed the title Wrapper has the same interface and internally calls the wrapped function by its name Wrapper has the same interface as the wrapped function Sep 13, 2015
@krlmlr
Copy link
Member Author

krlmlr commented Sep 20, 2015

Now complete, including revdep-check. If the memoized function is not yet defined during memoization, its formals cannot be queried, and the old implementation is used (with a warning). Ironically, the package that pointed me at that issue -- lubridate -- doesn't use memoise anymore. (It still fails, but this is due to locale settings on my machine.)

The individual merges (excluding trivial ones, oldest first) provide additional detail.

memoise <- memoize <- function(f) {
memoise <- memoize <- function(f, envir = parent.frame()) {
# We must not even try evaluating f -- once we start, there's no way back
if (inherits(try(eval.parent(substitute(f)), silent = TRUE), "try-error")) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you don't want to use exists here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think exists() won't work with anonymous functions.

Incorporate review notes
@krlmlr
Copy link
Member Author

krlmlr commented Sep 25, 2015

Thanks for the review, I have addressed all remarks. The most recent changes are https://github.com/krlmlr/memoise/pull/16/files.

@krlmlr
Copy link
Member Author

krlmlr commented Jan 13, 2016

Just updated the revdep check. Hope there won't be too many conflicts with @jimhester's pull requests.

@jimhester jimhester merged commit fd30983 into r-lib:master Jan 19, 2016
jimhester pushed a commit that referenced this pull request Jan 19, 2016
MINOR: fix for CRAN notes
@jimhester
Copy link
Member

Thanks Kirill, this makes the interface much nicer!

@jimhester jimhester mentioned this pull request Jan 22, 2016
@krlmlr krlmlr deleted the 12-interface branch January 29, 2016 09:53
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