Some fixes to make edit() and file.edit() more robust
#68
Conversation
| let uri = Url::from_file_path(&file); | ||
|
|
||
| let uri = unwrap!(uri, Err(_) => { | ||
| // The R side of this handles most issues, but we don't want to panic | ||
| // if some unknown file path slips through. | ||
| // `from_file_path()` doesn't return `Display`able errors, so we | ||
| // can't necessarily give a good reason. | ||
| log::error!("Can't open file at '{}'.", file); | ||
| return; | ||
| }); |
There was a problem hiding this comment.
As mentioned in the comment, it is fairly hard to get here now, but just in case we do we don't want to panic
| // `from_file_path()` doesn't return `Display`able errors, so we | ||
| // can't necessarily give a good reason. | ||
| log::error!("Can't open file at '{}'.", file); | ||
| return; |
There was a problem hiding this comment.
I see that this is an async function, and we use .await down below.
I don't need to do anything special before returning here, right? In my testing this seemed okay
| .ps.Call("ps_editor", as.character(file), as.character(title)) | ||
|
|
||
| options(editor = function(file, title, ..., name = NULL) { | ||
| handler_editor(file = file, title = title, ..., name = name) |
There was a problem hiding this comment.
I have put this in its own private helper instead, as that makes it easier to debug by name, i.e. debugonce(handler_editor)
| @@ -14,13 +14,69 @@ | |||
| } | |||
|
|
|||
| ensure_directory <- function(path) { | |||
| dir.create(path, showWarnings = FALSE, recursive = TRUE) | |||
There was a problem hiding this comment.
dir.create() is kind of lame unfortunately, since it only throws a warning if it can't create a directory, i.e. pass it dir.create("") for example.
I do think we want to error here, because we don't want to pass some invalid path to Rust
| # Try to create missing ones (`dir.create()` isn't vectorized) | ||
| for (elt in path) { | ||
| dir.create(elt, showWarnings = FALSE, recursive = TRUE) | ||
| } |
There was a problem hiding this comment.
i.e. for posit-dev/positron#857
| file.create(path, showWarnings = FALSE) | ||
|
|
||
| exists <- file.exists(path) |
There was a problem hiding this comment.
I was scared to use the result of file.create() to detect file existence because of this note in dir.create()
One of the idiosyncrasies of Windows is that directory creation may report success but create a directory with a different name, for example dir.create("G.S.") creates ‘"G.S"’.
So rather than using the creation result, we check for existence of that original path again
| title <- as.character(title) | ||
|
|
||
| # Get absolute path to file | ||
| file <- normalizePath(file, mustWork = FALSE) |
There was a problem hiding this comment.
This was previously path.expand() but that won't work for something like edit(file = "foo"). To get an absolute path we need normalizePath().
| if (!is.null(name)) { | ||
| stop("Editing objects is not currently supported.", call. = FALSE) | ||
| } |
There was a problem hiding this comment.
edit(mtcars) currently does work and goes through edit.data.frame(), but edit(1:5) goes through here and I've decided that we should just not allow this for now.
| if (identical(file, "")) { | ||
| # i.e. `edit()` with no arguments. Also `file.edit("")`. | ||
| # Opens a temporary file for editing. | ||
| file <- tempfile(fileext = ".txt") | ||
| } |
There was a problem hiding this comment.
This is documented behavior of ?edit
DavisVaughan
force-pushed
the
fix/edit-panic
branch
from
0d51d92 to
1f9c3b7
Compare
Addresses posit-dev/positron#856
Addresses part of posit-dev/positron#857 (we can open >1 file now, but
enablePreviewstill messes with it)Various improvements to make editing files through
edit()andfile.edit()more robust, I'll point them out inline@kevinushey do we have any testing infra for the public/private modules? It seems like it would be nice to add some kinds of tests for the error cases here at least, but I'm not sure where to put them or how they'd get run 😢