Bug 17403 - Patch to fix examples that write to working directory
Summary: Patch to fix examples that write to working directory
Status: CLOSED FIXED
Alias: None
Product: R
Classification: Unclassified
Component: Documentation (show other bugs)
Version: R-devel (trunk)
Hardware: Other Other
: P5 minor
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2018-04-04 03:24 UTC by Gabriel Becker
Modified: 2018-04-10 21:10 UTC (History)
2 users (show)

See Also:


Attachments
SVN diff file against r74516 (18.31 KB, patch)
2018-04-04 03:24 UTC, Gabriel Becker
Details | Diff
SVN diff file against r74516 (fixed) (18.83 KB, patch)
2018-04-04 04:14 UTC, Gabriel Becker
Details | Diff
SVN diff file against r74523 (2.02 KB, patch)
2018-04-04 17:08 UTC, Gabriel Becker
Details | Diff
Against R devel r74545 (12.21 KB, patch)
2018-04-07 11:00 UTC, Suharto Anggono
Details | Diff
Against R devel r74545 (12.24 KB, patch)
2018-04-07 11:12 UTC, Suharto Anggono
Details | Diff
Against R devel r74545 (12.24 KB, patch)
2018-04-07 11:16 UTC, Suharto Anggono
Details | Diff
Against R devel r74545 (12.26 KB, patch)
2018-04-07 11:23 UTC, Suharto Anggono
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Becker 2018-04-04 03:24:24 UTC
Created attachment 2332 [details]
SVN diff file against r74516

Attached is a patch which fixes many examples in the base packages which currently write directly to the working directory (now considered bad practice and disallowed for CRAN packages).

I explicitly chose not to use the 

   file(fil <- tempfile())
   <bla>
   unlink(fil)

opting to have an additional line where the fil variable is assigned, as I feel that this form is clearer for the novice R users who might be looking to the documentation to learn how to do things.

I cannot swear I found all the instances where this happens, but following Hadley's suggestion on the mailing list and a bit of extra snooping on my own I feel I found at least the majority of them.
Comment 1 Gabriel Becker 2018-04-04 04:14:45 UTC
Created attachment 2333 [details]
SVN diff file against r74516 (fixed)

Correct version of the patch which fixes examples in the base, grDevices, and utils packages to not write to a non-temp working directory.

The packages pass check and make check-devel runs all the examples without problem.
Comment 2 Martin Maechler 2018-04-04 09:14:48 UTC
Thank you, Gabe.  This looks good "from far"... I'll apply it and look a bit closer before committing {and possibly updating some *.Rout.save check files before that}.

There's been a lapsus in files.Rd (in the file.append() example), but I think there was a small need for change also in the non-unix case anyway.
Comment 3 Martin Maechler 2018-04-04 09:36:21 UTC
We have all the "file - based" graphics devices  which from their back compatibility with S and S+  have a default file name which in all cases is in the current working directory .. so one could argue  "not good".

You had a patch for pictex()  but not e.g. for postscript("foo.ps").
I'm leaving those alone entirely as I think we should really consider separately what to do there.

Another such example is  write()  which has a default  'file = "data"'  also from ca 30 years of compatibility with S,  but I think we should consider changing.
e.g. by considering  write.table()'s default, or *no* default. 
Definitely a different topic -- I think which should be discussed on the R-devel mailing list, rather than here.
Comment 4 Martin Maechler 2018-04-04 09:55:40 UTC
Last (?) comment:  I also did not apply the patch to  prompt.Rd and promptData.Rd

There again, there's an implicit default file name created in the current wd which has been "there" for ever i.e. ca 30 years, at least for prompt().

I think the example should start using

prompt(plot.default)   

or something similar without an explicit 'file = '.. notably as the function even talks to the user and explains about the file created.

Similar to the file-based graphics devices and write()  we may consider changing the default rather than the example...

and as there, such a change is definitely not for R 3.5.0,  whereas the remaining ones (I'll commit)  should also be ported.

Martin
Comment 5 Duncan Murdoch 2018-04-04 11:29:49 UTC
Regarding the prompt() example:  rather than changing prompt()'s default, or using the non-default file argument, I'd suggest wrapping the calls in a switch to the tempdir(), i.e. something like


  savedir <- setwd(tempdir())

  require(graphics)
  prompt(plot.default)
  prompt(interactive, force.function = TRUE)

  prompt(women) # data.frame

  prompt(sunspots) # non-data.frame data

  setwd(savedir)

Changing the default would make prompt() a lot less convenient.
Comment 6 Gabriel Becker 2018-04-04 17:08:01 UTC
Created attachment 2334 [details]
SVN diff file against r74523

I agree that the working directory approach is better for the prompt and graphics device examples. This patch implements that. It does not do so for the postscript example because it appears that that entire block is wrapped in \dontrun{} anyway. I can add it there too if preferred, but the working directory manipulation is wrapped in \dontshow{} anyway, so I figured there wasn't much reason to.

This also fixes an issue with my patch for files.Rd where the working directory wasn't changed back at the end of the example block.
Comment 7 Hadley Wickham 2018-04-04 17:56:34 UTC
FWIW CRAN will (sometimes?) run dontrun examples, so to be consistent, I think you should fix the postscript example too.
Comment 8 Martin Maechler 2018-04-05 13:47:11 UTC
Hadley is right: \dontrun{.} can be run and occasionally are in order to check they contain valid R code.

Still I had applied the 2334 patch.

Note that there's also  unix/png.Rd  and  windows/png.Rd
partly also use \dontrun{} example with files in "current dir" (not set to tempdir()).

This is not urgent though, and I think we can be content with the current changes for R 3.5.0 ...  and go further later. 
.. so I am keeping the status open for now.
Comment 9 Suharto Anggono 2018-04-07 11:00:23 UTC
Created attachment 2335 [details]
Against R devel r74545

I propose these changes. Global reasons:
- to be closer to the original
- for tempfile(), use 'fileext' argument only for file extension or suffix; desired name goes to 'pattern' argument
- consistency
- cosmetic
- for updated copyright year
Comment 10 Suharto Anggono 2018-04-07 11:12:25 UTC
Created attachment 2336 [details]
Against R devel r74545
Comment 11 Suharto Anggono 2018-04-07 11:16:01 UTC
Created attachment 2337 [details]
Against R devel r74545
Comment 12 Suharto Anggono 2018-04-07 11:23:02 UTC
Created attachment 2338 [details]
Against R devel r74545
Comment 13 Martin Maechler 2018-04-07 19:57:20 UTC
- I mostly agree,  particularly about a better use of  tempfile(*, fileext= *)
- I strongly prefer *.rda  to  *.RData so did change that change.

Thank you !

I'll try to port all these to R 3.5.0 now, as well.
Comment 14 Gabriel Becker 2018-04-08 15:11:38 UTC
No period is automatically added between the "filename" part of a tempfile and the fileext part, so changing, e.g., ".bz2" to "bz2" is incorrect. Otherwise these seem ok.
Comment 15 Martin Maechler 2018-04-10 21:10:10 UTC
(In reply to Gabriel Becker from comment #14)
> No period is automatically added between the "filename" part of a tempfile
> and the fileext part, so changing, e.g., ".bz2" to "bz2" is incorrect.
> Otherwise these seem ok.

I've just checked:  Suharto's patch did exactly do what's good:  *add* a leading "."  to all (but one ("-fifo"))  tempfile(fileext = ".<foo>")