Expand `strings_as_factors_linter()` to include `as.data.frame()` and `rbind.data.frame()`?

[IndrajeetPatil]

Since the same concerns apply to these functions as data.frame().

[AshesITR]

as.data.frame() seems like a good idea, I've never seen rbind.data.frame() called directly in the wild though.

[MichaelChirico]

we have rbind.data.frame() as an undesirable function IINM. technically it's exported so some users not understanding S3 dispatch may find it and call it directly.

I'd prefer not to encourage that, so I'd limit the extension to as.data.frame().

OTOH, the as.data.frame generic doesnt have stringsAsFactors as an argument, so I worry that will cause false positives.

BTW, could you give some examples where we could detect statically that stringsAsFactors should be included explicitly?

[IndrajeetPatil]

we have rbind.data.frame() as an undesirable function IINM

names(lintr::all_undesirable_functions)
#>  [1] ".libPaths"     "attach"        "browser"       "debug"        
#>  [5] "debugcall"     "debugonce"     "detach"        "ifelse"       
#>  [9] "library"       "loadNamespace" "mapply"        "options"      
#> [13] "par"           "require"       "sapply"        "setwd"        
#> [17] "sink"          "source"        "substring"     "Sys.setenv"   
#> [21] "Sys.setlocale" "trace"         "undebug"       "untrace"

^{Created on 2022-10-15 with reprex v2.0.2}

technically it's exported so some users not understanding S3 dispatch may find it and call it directly.

I've done this in the past 😭

BTW, could you give some examples where we could detect statically that stringsAsFactors should be included explicitly?

For example-

formals(as.data.frame.matrix)
#> $x
#> 
#> 
#> $row.names
#> NULL
#> 
#> $optional
#> [1] FALSE
#> 
#> $make.names
#> [1] TRUE
#> 
#> $...
#> 
#> 
#> $stringsAsFactors
#> [1] FALSE

as.data.frame(matrix("x"))
#>   V1
#> 1  x

^{Created on 2022-10-15 with reprex v2.0.2}

[MichaelChirico]

formals(as.data.frame.matrix)

formals(as.data.frame) does not share that parameter.

so the lint logic would have to consist of not just finding as.data.frame calls, but calls like as.data.frame(matrix(.)), as.data.frame(foo(.)), etc. it seems like a combinatorial explosion, and also not likely to catch all that much either TBH.

so I think: don't implement / out of scope

[IndrajeetPatil]

it seems like a combinatorial explosion, and also not likely to catch all that much either TBH

Fair enough.