xxx_dir(os) argument is broken

[jefferis]

The ability to specify an os other than the current one in the xxx_dir functions is fundamentally broken. This is because the internal file_path function uses normalizePath and path.expand, which have no notion of an os other than the current platform. For example on windows:

> user_data_dir(os='mac')
[1] "C:\\Users\\superFlyGuy\\Documents\\Library\\Application Support"

> path.expand("~")
[1] "C:\\Users\\superFlyGuy\\Documents"

I vote that we remove the os argument from all functions. It was introduced in @trevorld's huge 471aa1c commit without comment. I don't really see a strong case for wanting this. FWIW python appdirs does not have it https://github.com/ActiveState/appdirs/blob/master/appdirs.py

Removing os will in turn invalidate most of the tests, which can only be run in a platform specific way. The best that I think we can do is to run unix and windows tests based on the value of .Platform$OS.type. Likewise many examples will need to be changed to remove the os argument.

Alternatively, we could leave the os argument, but then it would only work for different flavours of unix and would have to error out otherwise. Or we could try to emulate normalizePath and path.expand with an os argument but that sounds like it would be costly and fragile and I don't see that the benefits warrant that effort.

Views @hadley? I think this is a blocker and you'll need to approve the strategy. @trevorld was there a particularly reason for the os argument that I'm missing? It looks like it may have been introduced for testing as much as anything.

[jefferis]

OK FWIW, jefferis#4, should fix this issue by removing the os arg, but it does inactivate a lot of tests on Windows. Sample winbuilder output:

http://win-builder.r-project.org/hrb3bZEdtZqU/examples_and_tests/

[trevorld]

On Jul 16, 2014 4:56 AM, "Gregory Jefferis" notifications@github.com wrote:

The ability to specify an os other than the current one in the xxx_dir functions is fundamentally broken. This is because the internal file_path function uses normalizePath and path.expand, which have no notion of an os other than the current platform. For example on windows:

user_data_dir(os='mac')
[1] "C:\Users\superFlyGuy\Documents\Library\Application Support"

path.expand("~")
[1] "C:\Users\superFlyGuy\Documents"

Not sure what you mean by "fundamentally broken": Under the "osx"
rules the data directory is placed in subfolder Application Support in
the subfolder "Library" of your home directory which is exactly what
it returns in your case. In the case of a Windows user looking at OSX
rules this indeed more useful for testing purposes than anything else.

I vote that we remove the os argument from all functions. It was introduced in @trevorld's huge 471aa1c commit without comment. @ I don't really see a strong case for wanting this. FWIW python appdirs does not have it https://github.com/ActiveState/appdirs/blob/master/appdirs.py

The os argument allows one to run cross-platform tests i.e. run all
the OSX tests on a linux machine or vice versa. That is a very
desirable feature to keep. Another, less compelling, use case is that
this would let OSX users read/write apps using the XDG standard (i.e.
writing an R function to read data created by some other Unix app that
when compiled on a Mac uses the XDG standard i.e. maybe writes into
.config). Although the os argument isn't in the python module doesn't
mean it shouldn't be in the R package (I also added a boolean argument
to allow R version expansion). The argument does come after all the
Python arguments and comes with the sensible default of using the
user's actual OS so the typical user never needs to mess with this
argument. However maybe it does make sense to preserve the os
argument for testing purposes but hide it from the user. One could
redefine the xxx_dir functions like `user_data_dir <- function(...)
{ user_data_dir_helper(..., os = get_os())} and then change the cross
platform unit tests to using the helper functions with different os
arguments.

Removing os will in turn invalidate most of the tests, which can only be run in a platform specific way. The best that I think we can do is to run unix and windows tests based on the value of .Platform$OS.type. Likewise many examples will need to be changed to remove the os argument.

Being able to run the OSX tests on a XDG machine or vice versa is
useful. As said by changing to helper function one can hide the
argument from the user if that is what Hadley really wants.

Alternatively, we could leave the os argument, but then it would only work for different flavours of unix and would have to error out otherwise. Or we could try to emulate normalizePath and path.expand with an os argument but that sounds like it would be costly and fragile and I don't see that the benefits warrant that effort.

Views @hadley? I think this is a blocker and you'll need to approve the strategy. @trevorld was there a particularly reason for the os argument that I'm missing? It looks like it may have been introduced for testing as much as anything.

While unaware of Hadley's effort I wrote a separate "appdirs" package.
The 'os' argument made it much much easier to test and hence develop
my version of the package on my Linux laptop. Since I found it useful
I kept the argument in my big package merge...

p.s. I'm on vacation with poor computer access but will return on July 28th.

[hadley]

I'd rather keep the os argument, since it makes cross-platform testing so much easier.

[jefferis]

I'd rather keep the os argument, since it makes cross-platform testing so much easier.

OK @hadley. I will make things work on that basis.

Just to be clear, this argument is useful for testing but not for end users. In fact it's probably dangerous for end users, because they might imagine that they would get the correct path used on another platform whereas they will get a frankenstein path if they are running on windows and asking for unix/mac or running on unix/mac and asking for windows.

@trevorld proposed hiding the argument by having user visible functions without os and hidden functions with os. Views?

Greg.

PS apologies, @trevorld, "fundamentally broken" is somewhat inflammatory language – it was not my intention to be rude. Sorry!

[jefferis]

NB I can make the tests work on Windows when os!='win'. ~ two thirds presently fail (sample winbuilder log) because the baseline paths in the tests asking for unix/mac dirs assume unix path conventions whereas the xxx_dir(os='unix/mac') returns a Windows path. So we have things like:

# user_data_dir(os='mac')
value="C:\\Users\\superFlyGuy\\Documents\\Library\\Application Support"

baseline="C:\\Users\\superFlyGuy\\Documents/Library/Application Support"

The fix is to wrap the baseline paths for each test in normalizePath or equivalent and results in the strange situation that many of the tests are checking whether a frankenstein path returned by code emulating the behaviour of another platform equals a baseline frankenstein path. This has now been proposed in #10

More generally my personal view is that the only reliable way to test Windows functionality is on Windows and the only reliable way to test unix functionality is on unix. That's what travis (now with multi-os (mac/linux) testing thanks to @krlmlr) and winbuilder do.

[jefferis]

Closed by #10 and #12