ATFutures / geoplumber

Serve geographic data from R and consume with scalable front end.
https://atfutures.github.io/geoplumber/
59 stars 7 forks source link

Switch default directory to tempdir() #34

Closed mpadge closed 6 years ago

mpadge commented 6 years ago

I appreciate that it's generally easier not to do this, but if this package is going to one day be sent to CRAN, then tempdir() is the only choice that will be accepted by them, so it may be easier to do this now? Feel free to disagree by closing this issue.

Robinlovelace commented 6 years ago

As long as it says where the app is located in a message, and can be overridden by user (features already in there I think) that sounds sensible to me.

layik commented 6 years ago

Over to you two! I am happy to to what it takes to get it on CRAN!

layik commented 6 years ago

So to clarify, now that I know what tempdir() does.

tempdir()
#> [1] "/tmp/RtmpokBhUk"

Created on 2018-09-18 by the reprex package (v0.2.0).

Are we saying CRAN will refuse code which creates a directory somewhere on the user's machine for the project? Because in its core, geoplumber assists a data scientist/developer to put together React (for now) and plumber to build something like ATT. Surely, they cannot force us to use tempdir for that?

What we have incidentally being discussing this morning with @Robinlovelace is where tempdir() fits perfectly and I was looking for it.

layik commented 6 years ago

A reference to CRAN's docs/view would be useful and will see if I can Google it up.

mpadge commented 6 years ago

Yeah, it's just like leaflet, which also simply dumps everything into tempdir(). That's the only way that CRAN will accept things, and we can just issue instructions for how to move the entire contents of that to some other place write all examples with non-default directories

layik commented 6 years ago

Thanks @mpadge just to make this concept crystal clear and indeed clarify where "geoplumber" stands. There was #9 which is now too old.

I can see two types of users for gp/geoplumber:

  1. One use case of geoplumber is someone like me who will do ATT again. 1.1 creates the web app and never uses pg again. 1.2 As the package grows (1.1) user might reconsider and say, well I can do quite a lot of what I would be doing manually so why not use geoplumber? 1.3 Then (1.1) and given (1.2) is available, geoplumber could be a development tool like devtools but for ATT like web apps.

  2. We also are thinking about a data scientist looking at some dataset and would do better with gp than with Shiny. Who might create a project, then get deleted in /tmp or using gp_erase(), start again next day etc.

I cannot think of other types of users at the moment but there could be more.

Now, are we managing working directory with the same workflow for both cases? Once I have a full understanding of yesterday/today's changes I could answer myself :)

mpadge commented 6 years ago

sorry for overlap with #9 - i hadn't seen that issue. (It was during my wonderful holiday, so I don't feel bad either!) I think that the wd workflow I've now implemented would apply equally to both use cases. Although the considerations of #9 remain entirely valid, my main concern in implementing this wd stuff was that the package as it stood would never have been accepted on CRAN. These wd manipulations are a necessary step to overcoming that, and I hope that I've implemented them in a way that mostly retains functionality the way you had it, and only changes bits that I subjectively see as improvements, such as users not having to manually change directories.

layik commented 6 years ago

Great, I thought there was some change but sounds like it is only to do it better (CRAN included).

As you might know, still not up to speed with this but but should be by this afternoon. Great work Mark.

mpadge commented 6 years ago

Maybe the best solution, contrary to how i named the issue, it to leave the default directory as is, but then to change all tests and examples to use tempdir(). (Now pretty easy with the changes I've implemented) The would then be acceptable on CRAN, and leave an easier default behaviour that wouldn't require users to manually copy stuff from tempdir(). Alternatively, we could use tempdir() throughout, and implement some additional kind of function that would do that moving itself? pb_move_dir()? Thoughts @layik @Robinlovelace?

layik commented 6 years ago

Maybe the best solution, contrary to how i named the issue, it to leave the default directory as is, but then to change all tests and examples to use tempdir().

This sounds to me to be the correct choice.

layik commented 6 years ago

Hi @mpadge. Is this done? I think I am now up to speed with the very useful util functions. I did put some comments on for my future reference.

layik commented 6 years ago

@mpadge this is still not 100% clear either in my head or parts of it still need to be implemented.

Do we then suggest that we need to switch to gp_create(tempdir()) or add necessary bits in gp_create() to use tempdir(). In both of these cases, if tempdir generates paths like this, then Create React App is not happy with it.

Initializing project at: /var/folders/z7/l4z5fwqs2ksfv22ghh2n9smh0000gp/T//RtmpTJV92H
Could not create a project called "RtmpTJV92H" because of npm naming restrictions:
mpadge commented 6 years ago

Yeah, that's exactly the kind of thing I was afraid of. This is going to take some work...

layik commented 6 years ago

Ah, glad we are on the same page.

Edit: Typo was almost causing a negation.

layik commented 6 years ago

Sorry to trouble @mpadge and @Robinlovelace

1.Now that #40 is out of my way I would like to finalise this. Clearly the Facebook npx command is not happy with tempdir() as it complains that "name can no longer contain capital letters". Can I just do tolower(tempdir()) for my "on the fly" functions like gp_sf()? I see no reason why not.

  1. And also, what else do I need to do to close this ticket?

Thanks.

Robinlovelace commented 6 years ago

Good work Layik. The tempdir() issue is a bit beyond my ken. If we're not including building of apps in CRAN tests I'm not sure current dir builds would be an issue but the tolower() option sounds like it may be viable, if a little 'bodgy'. :construction:

mpadge commented 6 years ago

Path specifications in most OS's, including generic windows, are not case sensitive. Windows can nevertheless be made case sensitive by simply setting the FILE_FLAG_POSIX_SEMANTICS variable. I've no idea whether or not this might be the case for CRAN test machines, but once we've got some tests actually running on Windows machines (and now that I've finally enabled Appveyor for our org), we should be able to tell. In short: You should be able to do this, and so I'd suggest just giving it a try. Please just add an appropriate comment in the code explaining why, and maybe a link to this issue.

mpadge commented 6 years ago

Here's a really nice example of default permanent directory with switch to tempdir() for tests, from the R.cache package.

layik commented 6 years ago

Thanks @mpadge and @Robinlovelace.

That R.cache example would need some time for my R muscles to digest. I have been playing with it but haven't got a push just yet.

layik commented 6 years ago

Another update.

tempdir() # works on all platforms with a platform-dependent result

I would love to tell the people on top of R foundation, that above comment should be in the description and not hidden by the examples of ?tempdir.

Anyways, to get tolower (which works fine) in the "tests" as per Mark's great supervision, I also need to cater for different platforms. I think "create" needs to look out for "tempdir" too but working on it.

layik commented 6 years ago

Almost there.

mpadge commented 6 years ago

This is great - I'll give it a good go-through as soon as I get a chance, but this looks like a great solution

layik commented 6 years ago

Thanks @mpadge removed the reference to the other issue as they are unrelated at this stage.

layik commented 6 years ago

this looks like a great solution

Mark, if not please re-open :)