utils::file.edit() silently fails in reprex() when reprex() is run in an RStudio session on Windows

[crew102]

• Issue related to line

  reprex/R/reprex.R

  Line 377 in 8c64e13

  withr::defer(utils::file.edit(reprex_file))

• This issue may have gone unnoticed thus far b/c utils::file.edit() is wrapped in defer() and defer() seems to muffle errors (CC'ing @kevinushey, as I'm not sure if this is desired behavior):

library(withr)
fo <- function() {
  defer({
    print("printing")
    stop("stoping")
  })
}
fo()
#> [1] "printing"

Created on 2018-09-13 by the reprex package (v0.2.0.9000).

• If reprex() didn't wrap utils::file.edit() in defer(), the user would see the following error to explain why no file had been opened:

some_file <- tempfile()
utils::file.edit(some_file)
#> Error in editor(file = file, title = title) : 
#>  argument "name" is missing, with no default

I believe this error stems from the fact that RStudio sets the "editor" option to be the following function on Windows:

https://github.com/rstudio/rstudio/blob/8af730409bb6d651cc8f6816d136bea91441e7a4/src/cpp/session/modules/SessionEnvironment.R#L379

.. but this function doesn't seem to play well with utils::file.edit() (it does work with RStudio's file.edit() though).

• Relevant info on the platform on which reprex() fails to open file:

R version: 3.4.4 (2018-03-15)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 7 x64 (build 7601) Service Pack 1
RStudio version: 1.1.419

• PR on the way

[jennybc]

I would appreciate your advice @kevinushey. It doesn't seem like a great idea for reprex to get deeper into this sort of business.

[kevinushey]

defer() explicitly catches errors because I was seeing some cases where R segfaults could occur when those errors were emitted. That said, it would probably be better for us to catch and re-emit those error messages as warnings.

IMHO this is something that should be fixed in RStudio directly, though.

[kevinushey]

That said, RStudio does this dance where we override the definition of file.edit() available on the search path, but not in the utils namespace. Which does unfortunately complicate the story for packages that want to use the 'appropriate' version of file.edit().

So a solution of this form might indeed be the best way forward.

[crew102]

@jennybc , can you expand on why you think reprex shouldn't include a fix for this issue?

[jennybc]

I feel differently now that @kevinushey has provided some context.

[kevinushey]

One other thing that's probably worth noting: in RStudio, a plain file.edit() just opens a file in an editor tab, whereas utils::file.edit() gives a modal editor (and so stops R code execution until the user has written to that file and closed it). In other words, utils::file.edit() is probably preferred if you need to read user input from a file; a plain file.edit() is better if you just need to open a file when all is said and done.

We should probably make these decisions concrete + apparent with helper functions in the rstudioapi package, though.

[jennybc]

But if I call a plain file.edit(), I get this in R CMD check:

N  checking R code for possible problems (2.9s)
   reprex: no visible global function definition for ‘file.edit’
   Undefined global functions or variables:
     file.edit
   Consider adding
     importFrom("utils", "file.edit")
   to your NAMESPACE file.

[kevinushey]

You could probably do something gross like:

file.edit <- if (exists("file.edit", envir = globalenv()))
  get("file.edit", envir = globalenv())
else
  utils::file.edit
file.edit(file)

[crew102]

In other words, utils::file.edit() is probably preferred if you need to read user input from a file; a plain file.edit() is better if you just need to open a file when all is said and done.

What would be the circumstance where reprex would be using file.edit to read user input? From my understanding, reprex will only be using file.edit to open files that it has written for the user so that the user can inspect them (post-execution of reprex()). In that case, I think using utils::file.edit() would be preferred (in those cases where it won't error, that is). See #205 for the logic I'm talking about.

[jennybc]

Having just re-learned this lesson the hard way, I think we also need to do this in a way where the "correct" version of file.edit() is selected at run time, not build time. So more along these lines:

file_edit <- function(...) {
  if (exists("file.edit", envir = globalenv())) {
    get("file.edit", envir = globalenv())(...)
  } else {
    utils::file.edit(...)
  }
}

@crew102 I agree that it seems like reprex's use of file.edit() is always as a way to give user access to something reprex has made versus user the other way around.

[crew102]

I think the version of file.edit() you have there should work for our purposes. I have one small bone to pick with it, though - I like how on OSX and Linux, utils::file.edit() opens up a modal dialog box with the contents of the file in it, instead of just opening up the file in a new RStudio tab (note that RStudio's file.edit() will only open a tab, never a modal). If you agree that opening up a modal should always be preferred to opening up a new tab, that means we need to use utils::file.edit() whenever possible (i.e., whenever it won't result in an error...it should only result in an error when we're on windows.)...As such, we'd have to use logic similar to what I have in #205 :

file_edit2 <- function(file) {
  if (is_windows()) {
    # this will return RStudio's file.edit if it exists...If it doesn't exist, it 
    # will return utils::file.edit()
    get("file.edit")(file)
  } else {
    utils::file.edit(file)
  }
} 

[jennybc]

Hmm ... I don't get the preference for a modal. If reprex has made something for the user (either a reprex or harvested and un-reprexed something from the web), it feels like they would want it in a "regular" tab instead of a modal that blocks all other activity, no?

[crew102]

Yeah, on second thought I guess it would make more sense to prefer tabs to modals... I was just considering the case where the user runs reprex and wants to view the code in the output file and/or copy and paste it from there. However, a regular tab makes more sense when the user is expected to interact with the code beyond doing a copy paste (which would be the case for the undo functions...for those functions, the user will probably want to run the code after it's opened).

[jennybc]

Let's go with the tab then. FYI I got a little patch release out this weekend, mostly because I have to give a webinar on Wednesday and I wanted a few small fixes to be out there.

I'd like the next release to really target some of this UI stuff: stress testing different workflows on Windows/RStudio Cloud, polish the addin, etc.

[krlmlr]

Much of this appears very similar to utils::View(), and tibble::view() that builds around it. I have started tidyview for a more robust view(), but the next version of tibble will include a very simple view() function.

Perhaps a robust implementation of file_edit() could be part of tidyview at some point?

[crew102]

Interesting package @krlmlr . I'd be happy to try to incorporate file_edit() into tidyview. Let's pick up discussion in krlmlr/tidyview#1.