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

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

Closed
cgiachalis opened this issue Aug 26, 2019 · 3 comments
Closed

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

cgiachalis opened this issue Aug 26, 2019 · 3 comments
Labels
question User questions and open questions about development
Milestone

Comments

@cgiachalis
Copy link
Contributor

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 joshuaulrich added the question User questions and open questions about development label Aug 26, 2019
@joshuaulrich
Copy link
Owner

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
Copy link
Contributor Author

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 joshuaulrich added this to the 0.12.3 milestone Oct 12, 2022
@joshuaulrich
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question User questions and open questions about development
Projects
None yet
Development

No branches or pull requests

2 participants