Add encoding support #518
Add encoding support #518
Conversation
…ompatability on Windows (see e.g. r-lib/devtools#1306)
|
Until this is merged, Can you please consider reviewing it? |
|
@gaborcsardi: Did you mean the |
| @@ -1,4 +1,5 @@ | |||
| # testthat 1.0.2.9000 | |||
| * New argument`encoding` in `test_file()` and `source_file()` (@hansharhoff, devtools#1306) | |||
There was a problem hiding this comment.
Did you mean to refer to r-lib/devtools#1306?
There was a problem hiding this comment.
Yes,
is this better?
(@hansharhoff, hadley/devtools#1306)
| } | ||
|
|
||
| test_files <- function(paths, reporter = "summary", | ||
| test_files <- function(paths, reporter = "summary", encoding = "unknown", |
There was a problem hiding this comment.
How are the dots ... used in this function?
There was a problem hiding this comment.
I am not sure I completely understand. I wanted to keep it as close as possible to the parent.
There was a problem hiding this comment.
The dots ... currently are not used by test_files(). They weren't before, but as we're touching the code now, it's better to clean this up.
| @@ -98,11 +99,14 @@ find_test_scripts <- function(path, filter = NULL, invert = FALSE, ...) { | |||
| #' @param reporter reporter to use | |||
| #' @param env environment in which to execute the tests | |||
| #' @param load_helpers Source helper files before running the tests? | |||
| #' @param encoding the encoding of the files in test directory. Default is | |||
There was a problem hiding this comment.
Perhaps shorter: File encoding, default: "unknown"
…ompatability on Windows (see e.g. r-lib/devtools#1306) Add NEWS.md line about the change.
hansharhoff
force-pushed
the
add-encoding-support
branch
from
8dbe20b to
34f4565
Compare
|
I believe I addressed the points of @krlmlr with this commit. If you agree I will ping hadley. |
| } | ||
|
|
||
| test_files <- function(paths, reporter = "summary", | ||
| test_files <- function(paths, reporter = "summary", encoding = "unknown", |
There was a problem hiding this comment.
The dots ... currently are not used by test_files(). They weren't before, but as we're touching the code now, it's better to clean this up.
Remove ... from call/definition of test_files
|
@krlmlr , I have removed the dots and simplified the arguments. I take it those were the open comments. |
| @@ -98,11 +99,14 @@ find_test_scripts <- function(path, filter = NULL, invert = FALSE, ...) { | |||
| #' @param reporter reporter to use | |||
| #' @param env environment in which to execute the tests | |||
| #' @param load_helpers Source helper files before running the tests? | |||
| #' @param encoding File encoding, default is "unknown" | |||
There was a problem hiding this comment.
Can you use @inheritParams source_file here?
|
@hadley: PTAL. |
| stopifnot(file.exists(path)) | ||
| stopifnot(is.environment(env)) | ||
|
|
||
| lines <- readLines(path, warn = FALSE) | ||
| lines <- readLines(path, warn = FALSE, encoding = encoding) |
There was a problem hiding this comment.
The encoding argument does not do what you think it does...
This pull request adds support for an encoding argument to test_dir (and the functions called by this). This is related to an issue reported in devtools. A separate pull request is incoming for devtools.