With use_test(), if the current source editor file name is, e.g. camel case, the test-*.R file that is created is converted to all lower case with a call to tolower(). I think this is making an unnecessary decision for the user. Additionally, if the user tries to force the file name by passing a name = argument, this is also converted to lower case.
Example of the problem with default:
source editor filename: "my_coolFunctionPrint.R"
use_test()
resulting test: "test-my_coolfunctionprint.R"
Example of the problem with name = *** passed:
source editor filename: "my_coolFunctionPrint.R"
use_test("theBestFunctionEver")
resulting test: "test-thebestfunctionever.R"
This is likely unnoticed if strict adherence to the Tidyverse style guide, but other style guides will break the filename.R -> test-filename.R pattern. There appears to be a tolower() call somewhere converting the case that needs some conditional logic to it. I'd be happy to submit a PR if necessary.
@batpigandme
I'll take a stab at this one ... looks like the magic happens in the asciify() function within utils.R
@stufield do you still want to tackle it? I'm also happy to do it if you don't have time.
I actually already did it in my forked branch, but thought it might be rude to submit an unsolicited PR before you had a chance to deem it worthy.
asciify <- function(x) {
stopifnot(is.character(x))
#x <- tolower(x)
gsub("[^a-zA-Z0-9_-]+", "-", x)
}
This is all I did, very simple. Only trick is you must update the gsub() call to now include capitals since the tolower() is gone.
Happy tol submit the PR if you haven't already done it.
Hmmmm, why are we calling asciify() at all? Maybe the problem is that we actually need a completely different branch if the path comes from an existing file?
I am not 100% sure I follow. Do you mean coming from an existing (open) R script, or from an existing test-*.R file? I think this branch is created on Line 34 where if name is NULL we create a name from the existing file with get_active_r_file(). Then below, if that file already exists, we simply open it. Otherwise we create it with use_template(). However, I have a feeling I've misunderstood what you mean.
My understanding of asciify() is to stop users from introducing any non-ascii characters that may not be cross-platform friendly. Wouldn't this be the case no matter what? Or, are you saying that IF the user has some non-ascii file name (either the test-*.R file or the open RStudio file), we shouldn't be making this decision for them?
There are two ways to get a name for the test file:
User supplied - the user could type anything, so we need to make sure it's a valid file name. (Although maybe rather than trying to fix up a bad name we should just throw an error)
From an existing open file, which implies there's already a valid file name and we don't need to check.
i.e. maybe use_test() should just do:
if (is.null(name)) {
name <- get_active_r_file(path = "R")
} else {
check_file_name(name)
}
Do you think it's safe to assume that if the file already exists it is valid? It might be safer not to assume anything and check in either case. Perhaps softening things with a warning if the existing file is invalid.
Maybe we could just do something like:
If user supplied, check with check_file_name() and throw error ... bad name = argument passed.
If existing file, check with valid_path_name(), and trip a warning suggesting to change the file to something safer.
This satisfies your original intuition, we don't need to be calling asciify() at all.
Yeah, I think it's fine to skip the check if the file already exists.
Ha ha! I actually came around to your original instinct once I got into writing it. Does it really make sense to be creating a file called test-$ome#fun+I_like.R? So I went with with checking both cases. Perhaps as this point I should submit a PR and give you an opportunity to actually look at the code? Would you prefer that?
Yeah, that'd be great.