Make accept_snapshot() work on filenames containing a dot (#1669) #2029
Conversation
|
The test failures seem unrelated: I also get them locally on the main branch when using |
This changes slightly the logic for matching file names: - before if a file name was considered to be without extension, we would add the `.md` and then compare it with the actual file names in the `_snaps` directory. However, that would fail if the file contained dots, as they would be considered to be a file extension: for them no `.md` was added to their name, and therefore they would never match with the actual files in the `_snaps` directory, so no update could ever succeed - with this patch, we strip any `.md` or `.txt` extension from the file names received by `snapshot_manage()`, and compare those with the actual file names from the `_snaps` directory also without extension
|
I've rebased so that the NEWS entry is in the correct place. |
|
Can anybody review this? It comes with additional tests, it passes the existing ones, and fixes a real bug. @hadley |
|
@mcol sorry, I'm busy with other projects at the moment. I'll definitely look at it when I'm next working on testthat. Thanks for your patience! |
R/snapshot-manage.R
Outdated
| @@ -161,9 +161,11 @@ snapshot_meta <- function(files = NULL, path = "tests/testthat") { | |||
| files <- files[!is_dir] | |||
|
|
|||
| dirs <- substr(dirs, 1, nchar(dirs) - 1) | |||
| files <- ifelse(tools::file_ext(files) == "", paste0(files, ".md"), files) | |||
| files <- gsub("\\.md|\\.txt$", "", files) | |||
There was a problem hiding this comment.
I think your fix assumes that snapshots can only have file names ending in .md or .txt, but in fact they can have any extension because the user supplies the extension in expect_snapshot_file().
But overall I find this code (i.e. the code that existed before your PR) very hard to understand: I don't understand what it's try is trying to achieve, or how the logic flows. So that might imply it's time to take a step back and rewrite this code to make it more clear what the varies inputs are.
There was a problem hiding this comment.
I've pushed a partial fix for the hardcoded extensions, although it may not work if there are multiple snapshot extensions being used simultaneously (but also, there's no pre-existing test for that case, independently of dots in the filenames).
Having said that, I agree that this code is quite hacky as it is. Are you planning to do it or should I have a go?
There was a problem hiding this comment.
I'd really appreciate it if you wanted to have a go 😄
Well, this still assumes that there is just one extension being used among all snapshot files.
This changes slightly the logic for matching file names:
before if a file name was considered to be without extension, we would add the
.mdand then compare it with the actual file names in the_snapsdirectory. However, that would fail if the file contained dots, as they would be considered to be a file extension: for them no.mdwas added to their name, and therefore they would never match with the actual files in the_snapsdirectory, so no update could ever succeedwith this patch, we strip any
.mdor.txtextension from the file names thatsnapshot_manage()receives, and compare those with the actual file names from the_snapsdirectory also without extensionI've added tests and a NEWS item (all tests pass locally).
Fixes #1669.