Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some fixes to make edit() and file.edit() more robust #68

Merged
merged 2 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions crates/ark/src/lsp/editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use harp::vector::CharacterVector;
use harp::vector::Vector;
use libR_sys::*;
use stdext::unwrap;
use tokio::runtime::Runtime;
use tower_lsp::lsp_types::ShowDocumentParams;
use tower_lsp::lsp_types::Url;
Expand All @@ -19,13 +20,25 @@ unsafe extern "C" fn ps_editor(file: SEXP, _title: SEXP) -> SEXP {
let rt = Runtime::new().unwrap();
let globals = R_CALLBACK_GLOBALS.as_ref().unwrap();
let files = CharacterVector::new_unchecked(file);

for file in files.iter() {
if let Some(file) = file {
rt.block_on(async move {
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
Comment on lines +27 to +36
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


globals
.lsp_client
.show_document(ShowDocumentParams {
uri: Url::from_file_path(file).unwrap(),
uri,
external: Some(false),
take_focus: Some(true),
selection: None,
Expand All @@ -36,5 +49,5 @@ unsafe extern "C" fn ps_editor(file: SEXP, _title: SEXP) -> SEXP {
}
}

file
R_NilValue
}
35 changes: 35 additions & 0 deletions crates/ark/src/modules/private/options.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#
# options.R
#
# Copyright (C) 2023 Posit Software, PBC. All rights reserved.
#
#

handler_editor <- function(file, title, ..., name = NULL) {
# `file.edit()` calls this as `editor(file = file, title = title)`
# `edit()` calls this as `editor(name = name, file = file, title = title)`

if (!is.null(name)) {
stop("Editing objects is not currently supported.", call. = FALSE)
}
Comment on lines +12 to +14
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
}
Comment on lines +16 to +20
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is documented behavior of ?edit


file <- as.character(file)
title <- as.character(title)

# Get absolute path to file
file <- normalizePath(file, mustWork = FALSE)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously path.expand() but that won't work for something like edit(file = "foo"). To get an absolute path we need normalizePath().


# Make sure the requested files exist, creating them if they don't
ensure_file(file)

# Edit those files.
.ps.Call("ps_editor", file, title)

invisible()
}
58 changes: 57 additions & 1 deletion crates/ark/src/modules/private/tools.R
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,69 @@
}

ensure_directory <- function(path) {
dir.create(path, showWarnings = FALSE, recursive = TRUE)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

exists <- dir.exists(path)

if (all(exists)) {
# Nothing to do if they all exist
return(invisible())
}

path <- path[!exists]

# Try to create missing ones (`dir.create()` isn't vectorized)
for (elt in path) {
dir.create(elt, showWarnings = FALSE, recursive = TRUE)
}
Comment on lines +26 to +29
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


exists <- dir.exists(path)

if (all(exists)) {
# Nothing left to do if the missing ones were successfully created
return(invisible())
}

path <- path[!exists]
path <- encodeString(path, quote = "\"")
path <- paste0(path, collapse = ", ")

stop("Can't create the directory at: ", path, call. = FALSE)
}

ensure_parent_directory <- function(path) {
ensure_directory(dirname(path))
}

ensure_file <- function(path) {
exists <- file.exists(path)

if (all(exists)) {
# All files exist already, nothing to do
return(invisible())
}

path <- path[!exists]

# Create parent directories as needed
ensure_parent_directory(path)

# Try to create the missing files
file.create(path, showWarnings = FALSE)

exists <- file.exists(path)
Comment on lines +63 to +65
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (all(exists)) {
# We successfully created the new files and can detect
# their existance
return(invisible())
}

path <- path[!exists]
path <- encodeString(path, quote = "\"")
path <- paste0(path, collapse = ", ")

stop("Can't create the files at: ", path, call. = FALSE)
}

# Checks if a package is installed without loading it.
# Could be slow on network drives.
is_installed <- function(pkg, minimum_version = NULL) {
Expand Down
12 changes: 2 additions & 10 deletions crates/ark/src/modules/public/options.R
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,8 @@ options(max.print = 1000)
options(help_type = "html")

# Use internal editor
options(editor = function(file, title, ...) {

# Make sure the requested files exist.
file <- as.character(path.expand(file))
ensure_parent_directory(file)
file.create(file[!file.exists(file)])

# Edit those files.
.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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have put this in its own private helper instead, as that makes it easier to debug by name, i.e. debugonce(handler_editor)

})

# Use custom browser implementation
Expand Down