joshuaulrich / xts

Extensible time series class that provides uniform handling of many R time series classes by extending zoo.
http://joshuaulrich.github.io/xts/
GNU General Public License v2.0
220 stars 71 forks source link

`na.locf` and the use of `stopifnot(is.xts(object))` #307

Closed cgiachalis closed 1 year ago

cgiachalis commented 5 years ago

Do we need stopifnot(is.xts(object)) in a xts method?

body(xts:::na.locf.xts)
{
    stopifnot(is.xts(object))
    maxgap <- min(maxgap, NROW(object))
    if (length(object) == 0) 
        return(object)
    if (hasArg("x") || hasArg("xout")) 
        return(NextMethod())
    x <- .Call("na_locf", object, fromLast, maxgap, Inf, 
        PACKAGE = "xts")
    if (na.rm) {
        return(structure(na.omit(x), na.action = NULL))
    }
    else x
}
sessionInfo()
R version 3.6.0 (2019-04-26)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 7 x64 (build 7601) Service Pack 1

Matrix products: default

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] xts_0.11-2 zoo_1.8-6 

loaded via a namespace (and not attached):
 [1] compiler_3.6.0    assertthat_0.2.1  cli_1.1.0         tools_3.6.0      
 [5] withr_2.1.2       rstudioapi_0.10   crayon_1.3.4      grid_3.6.0       
 [9] packrat_0.5.0     lattice_0.20-38   sessioninfo_1.1.1
joshuaulrich commented 5 years ago

No, you generally don't need to check that the class of the object matches the class of the method. That said, I do notice people often call methods directly, especially if they're exported. This is likely due to auto-completion in their IDE (e.g. RStudio).

That line of code has been there for ~10 years now. I'm not inclined to remove it unless there's a very good reason. Is this causing an issue for you, or are you asking because you're curious?

cgiachalis commented 5 years ago

Thanks Joshua.

No, it's not an issue at all. It's more a question of consistency, i.e., na.omit() or split() do not check that the class of the object matches the class of the method.

joshuaulrich commented 1 year ago

There's no need to include this line in an unexported method. I'll remove it. It was added in early 2009, before R required packages to have namespaces... in late 2011. Ooof, feeling old.