Fix issues with using absolute paths for modules

[torfason]

It seems file.path() is not as smart as I thought about absolute paths. When given an absolute path relative to ".", it converts it into an unusable relative path. I added a new function, file_path(), which returns absolute paths unchanged.

Credit to Henrik Bengtson and the R.utils package for the isAbsolutePath() function which does some heavy lifting to determine on multiple OSs whether a path is absolute. I copied the function rather than importing the whole package, which felt like overkill.

This also allowed me to add tests for downloading URLs using the pins package (the local file name returned by pins::pin() is an absolute file name.

[smbache]

interestingly the CI doesn't run here - I think if you add pull_request to the on array in the actions yml, it may trigger correctly?

[smbache]

can you give an example of where file.path is unpredictable? is it a windows thing?

[torfason]

file.path(".","/dev/null")
#> [1] ".//dev/null"
import:::file_path(".","/dev/null")
#> [1] "/dev/null"

Not so much that file.path()is unpredictable, just that I didn't predict it. It converts all absolute paths into relative paths when prefixed with "." to implement the .directory parameter, I thought that it was cleanest to use "." as a default, requiring the use of afile.path()variant. But that caused issues with absolute paths, and figuring out whether a path is absolute turns out to be nontrivial.

Now that I think about it, it might be possible to simply rewrite file_path() to return the original file name whenever the first argument is "." instead of whenever the second argument is an absolute path. If that works, that would alleviate the need to pull in this huge function ...

[torfason]

If I redo the pull request fresh from master, a side effect might be to have the tests pass. It might be something about how the CI confiq was merged into the pull request after it was created ...

[smbache]

Is it cleaner to just use getwd() instead of using "."?

Re the CI, I think if you change

on: [push]

to

on: [push, pull_request]

it should pick it up.. just a guess :) no harm trying.

[torfason]

I'm afraid getwd() wont help:

# file.path()
file.path(".","/dev/null")
#> [1] ".//dev/null"
file.path(getwd(),"/dev/null")
#> [1] "/Users/torfason/demodir//dev/null"

# Contrast with import::file_path()
import:::file_path(".","/dev/null")
#> [1] "/dev/null"
import:::file_path(getwd(),"/dev/null")
#> [1] "/dev/null"

And on closer thought I'm not too fond of special handling of ".". What if someone has most of his helpers in `~/utils', but then needs to pass one helper though absolute path:

import::from(util.R, things, .directory="~/utils")
import::from("/shared/util.R", shared_things, .directory="~/utils") # error (this could be inside a function, using character_only

It seems to be an effort to explain to that person that they need to delete .directoryor set to "." to trigger some special handling (I guess they could set to "/" for absolute files, but that would require them do do the heavy lifting of figuring out if the script is an absolute path.

So I'm back to thinking isAbsolutePath() from R.utils is the best option, even if it is big it is presumably decently tested upstream, and passes all tests on all platforms (windows as well, it is just the CI that has issues).

I'll fix other issues, including the attempted fix for CI, and update the PR.

[torfason]

I was forced to remove pins from suggests, because Ubuntu/Windows tests were failing. Seems like those configurations are in general more finicky than osx (or I'm just biased because I work on osx myself). But better to run into those issues immediately on push to GitHub than run into them on CRAN submission or through bug reports from linux/win users.

[torfason]

@smbache, it would be great to get your thoughts on whether you think it's OK to merge this pull request into master (I can then perform the actual merge). Would be good to clear this out of the way before preparing a PR to fix the recursive call issues.