insertPackage should work with absolute filenames? about drat HOT 15 CLOSED

Yes indeed it should. I must have busted that when generalizing for git(hub) use. We need to look at srcdir and repodir -- do you have a few spare cycles to look at it?

from drat.

I'll give it a try; my inner perl hacker (ugh) says to rewrite

file.copy(file.path(curwd, file), srcdir, overwrite=TRUE)

as

file.copy(file.path(curwd, file), srcdir, overwrite=TRUE) || file.copy(file, srcdir, overwrite=TRUE)

from drat.

oh, and maybe wrap that all with stopifnot.

from drat.

I'd pre-test with

if (!file.exists(file))  {  whine() }

etc

from drat.

There is already a check for the file at the head of the function.

from drat.

It's not quite right.

    stopifnot(file.copy(file.path(curwd, file), srcdir, overwrite=TRUE) || 
                        file.copy(file, srcdir, overwrite=TRUE))

If we are in curwd, then both branches are equivalent, so this does not yet help enough. I'll look at it some more on the train home in a few hours.

from drat.

It might help if you convert the path to absolute in the first place. E.g.

absolute_path <- function(path) {
  path <- path.expand(path)
  if (substr(path, 1, 1) == "/") {
    path
  } else {
    file.path(getwd(), path)
  }
}

Btw. why can't you just say file.copy(file, srcdir, ...)? I haven't read the complete code, so maybe this is not an option.

from drat.

Nice idea, thanks.

It should actually be pretty simple. file.copy() surely takes anything, absolute or relative, but I need a partial path later in the (optional) git2r::add() call, and that is where I went off the road prior to 0.0.2.

from drat.

The file.copy(file.path(curwd, file), srcdir) is wrong, and it arose because of the optional setwd() earlier.

from drat.

I'm not sure both "both branches are equivalent":

> file.path(getwd(),'/tmp/foobar.tar.gz')
[1] "/home/spav//tmp/foobar.tar.gz"

but you're right, I forget to fix the git2r::add.

from drat.

The stopifnot( ... || ...) is still weird, and I don't like it. We are in curwd, so both are the same. At the most, use explicit tests and stop() calls.

The basename() looks good.

from drat.

Ack re your comment of both branches being different. The problem was the thinko related to the earlier setwd(). At that point all we need is file.copy(file, dir) having tested that both file and dir exist.

from drat.

This may actually be much less scary than we thought. Can you look at what I just committed? Either via checkout, or by simply pulling the relevant file as is ?

from drat.

LGTM for absolute paths.

Should there be a catch on file.copy returning `FALSE`` (e.g. due to permissions issues)?

from drat.

Great, and a) sorry for the earlier runaround and b) yes that is a very good idea. Help page suggest that a boolean is returned to lemme add that.

from drat.