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

Don't use . to separate function names #2

Open
hadley opened this issue Jun 16, 2015 · 6 comments
Open

Don't use . to separate function names #2

hadley opened this issue Jun 16, 2015 · 6 comments

Comments

@hadley
Copy link

hadley commented Jun 16, 2015

It's probably too late to fix this, but in R, it's conventional to use . only for S3 method names.

@yixuan
Copy link
Owner

yixuan commented Jun 16, 2015

Hmm, that's what I really regret. Any suggestion to fix this but not to break the API? The only thing I can think of is to add underscore aliases and deprecate the dot ones.

@hadley
Copy link
Author

hadley commented Jun 17, 2015

Yeah, I think that would be the best approach.

@DaveParr
Copy link

I see you've implemented the best practice my_fun(), and also have a useful message to that effect: 'showtext.begin()' is now renamed to 'showtext_begin()' The old version still works, but consider using the new function in future code.

However, I think that you are also triggering your own error message by not adopting this practice in your code behind the scenes. I am only using fig.showtext = TRUE, and showtext_auto() in my script, and receiving that warning. Are lines such as showtext::showtext.begin triggering your own warnings?

showtext.auto = function(enable = TRUE)
{
    enable = as.logical(enable)
    
    has_hook = length(getHook("before.plot.new")) > 0
    is_showtext_hook = sapply(getHook("before.plot.new"), identical,
                              y = showtext::showtext.begin)
    
    has_hook_grid = length(getHook("grid.newpage")) > 0
    is_showtext_hook_grid = sapply(getHook("grid.newpage"), identical,
                                   y = showtext::showtext.begin)

    already_hooked = has_hook && any(is_showtext_hook)
    already_hooked_grid = has_hook_grid && any(is_showtext_hook_grid)
    
    if(enable)
    {
        if(!already_hooked)
            setHook("before.plot.new", showtext::showtext.begin)
        if(!already_hooked_grid)
            setHook("grid.newpage", showtext::showtext.begin)
    } else {
        if(already_hooked)
        {
            old_hooks = getHook("before.plot.new")
            new_hooks = old_hooks[!is_showtext_hook]
            setHook("before.plot.new", new_hooks, "replace")
        }
        if(already_hooked_grid)
        {
            old_hooks = getHook("grid.newpage")
            new_hooks = old_hooks[!is_showtext_hook_grid]
            setHook("grid.newpage", new_hooks, "replace")
        }
    }
}

@yixuan
Copy link
Owner

yixuan commented Jan 15, 2018

Good catch. In fact I have updated the code on CRAN, but I forgot to update github. The real reason is that in knitr I still call showtext.begin() instead of showtext_begin(). I'll create a patch to knitr later this week.

Also @yihui.

@yihui
Copy link

yihui commented Jan 15, 2018

I have changed it in knitr a while ago (the CRAN version of knitr should be using showtext_begin()):
https://github.com/yihui/knitr/blob/4d71624d8ab9cac51a2245dc97d7c54b5c29eb68/R/plot.R#L317

@yixuan
Copy link
Owner

yixuan commented Jan 15, 2018

@yihui I see. I didn't check the code carefully.

So there should be no code calling the old functions. Could you double check your issue? @DaveRGP

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

No branches or pull requests

4 participants