ices-tools-prod / TAF

Transparent Assessment Framework for Reproducible Research
GNU General Public License v3.0
3 stars 2 forks source link

taf.skeleton() should have an argument that puts icesTAF as the default package. #9

Closed colinpmillar closed 1 year ago

colinpmillar commented 1 year ago

https://github.com/ices-tools-prod/TAF/blob/67cc1c6483efe0f462b05cb5d63196673ea1daf5/R/taf.skeleton.R#L42

By far the widest users of TAF are ices scientist and they are being taught to use icesTAF. I suggest that the function has an arg to select which base package to use. various optoins for argument names, user = "ices_user", "other", or base = "icesTAF", or "TAF" .

Ta!

arni-magnusson commented 1 year ago

Well spotted! Will set the default as library(icesTAF).

colinpmillar commented 1 year ago

Or! maybe I could put a taf.skeleton() 'alias' in icesTAF ? would that work?

arni-magnusson commented 1 year ago

Good question, I'm not entirely sure.

My guess is that an alias might produce a warning every time a user or script runs library(icesTAF), right? That kind of 'noise pollution' might be seen as a somewhat high cost...

It would be worth exploring this to find out whether an alias would always produce a warning, since situations like this may arise elsewhere.

For the specific case of taf.skeleton(), setting the default to icesTAF is only a minor iconvenience for non-icesTAF users. Another way could be to set it to icesTAF if that package is currently loaded? Perhaps TAF could have the following function to resolve whether to apply ICES things:

icesTAF <- function()
  "package:icesTAF" %in% search()
colinpmillar commented 1 year ago

So I have already re-exported all the TAF functions in icesTAF to solve a load of backwards compatability issues, and I dont get any warnings.

The issue would only be if I have a diferent implementation of taf.skeleton(), but I htink I'd be OK. It would only be an issue if you load library(TAF) after library(icesTAF) perhaps.

arni-magnusson commented 1 year ago

Hey, that sounds promising. We could alias taf.skeleton() in the icesTAF package and leave it unchanged in the TAF package?

That's smoother than I thought. I'm almost sure if a package provides a function with the same name as a base function, effectively overriding it, this produces a message to warn the user that "The following object is masked from 'package:stats':" when loading that package.

Is this message not triggered when overriding a non-base package, such as TAF, or is alias different from overriding?

colinpmillar commented 1 year ago

I think it will work. Here how it looks when you have identical functions:

r$> library(TAF)

r$> library(icesTAF)

r$> taf.skeleton
function (path = ".", force = FALSE)        
{
    safe.cat <- function(..., file, force) {
        if (!file.exists(file) || force) {  
            cat(..., file = file)
        }
    }
    mkdir(path)
    owd <- setwd(path)
    on.exit(setwd(owd))
    mkdir("bootstrap/initial/data")
    template <- paste0("## %s\n\n## Before:\n## After:\n\n",
        "library(TAF)\n\nmkdir(\"%s\")\n\n")
    headers <- list(data = "Preprocess data, write TAF data tables",
        model = "Run analysis, write model results", output = "Extract results of interest, write TAF output tables",   
        report = "Prepare plots and tables for report")
    for (section in names(headers)) {
        safe.cat(sprintf(template, headers[[section]], section),
            file = paste0(section, ".R"), force = force)
    }
    invisible(getwd())
}
<bytecode: 0x00000208862ffdd0>
<environment: namespace:TAF>

r$> icesTAF::taf.skeleton
function (path = ".", force = FALSE)        
{
    safe.cat <- function(..., file, force) {
        if (!file.exists(file) || force) {  
            cat(..., file = file)
        }
    }
    mkdir(path)
    owd <- setwd(path)
    on.exit(setwd(owd))
    mkdir("bootstrap/initial/data")
    template <- paste0("## %s\n\n## Before:\n## After:\n\n",
        "library(TAF)\n\nmkdir(\"%s\")\n\n")
    headers <- list(data = "Preprocess data, write TAF data tables",
        model = "Run analysis, write model results", output = "Extract results of interest, write TAF output tables",   
        report = "Prepare plots and tables for report")
    for (section in names(headers)) {
        safe.cat(sprintf(template, headers[[section]], section),
            file = paste0(section, ".R"), force = force)
    }
    invisible(getwd())
}
<bytecode: 0x00000208862ffdd0>
<environment: namespace:TAF>
colinpmillar commented 1 year ago

i'll have a think and a try and get back to you :)

colinpmillar commented 1 year ago

hi, so I have made TAF an import rather than a depends and you dont get any warnings, yet the function defs and namespace still comes from TAF. so all good.

arni-magnusson commented 1 year ago

Resolved in commits feac139 (TAF) and a6791be (icesTAF).