stplanr

[Robinlovelace]

• 1. What does this package do? (explain in 50 words or less)
     This package brings together a number of tools that enable transport modelling and planning and R, with an emphasis on analyses needed for sustainability (e.g. planning new bicycle paths).
• 2. Paste the full DESCRIPTION file inside a code block (bounded by ``` on either end).

Package: stplanr
Type: Package
Title: Sustainable Transport Planning with R
Version: 0.0.1.1
Date: 2015-10-18
Authors@R: c(
    person("Robin", "Lovelace", email = "rob00x@gmail.com", role = c("aut", "cre")),
    person("Richard", "Ellison", role = c("aut"), comment = "Author of various functions"),
    person("Barry", "Rowlingson", role = c("aut"), comment = "Author of overline"),
    person("Nick", "Bearman", role = c("aut"), comment = "Co-author of gclip")
    )
Description: Functionality and data access tools for transport planning, including origin-destination analysis, route allocation and modelling travel patterns.
License: MIT + file LICENSE
BugReports: https://github.com/robinlovelace/stplanr/issues
LazyData: yes
Depends:
    sp, R (>= 3.0)
Imports:
    jsonlite,
    maptools,
    raster,
    rgdal,
    rgeos,
    dplyr,
    RgoogleMaps,
    openxlsx,
    leaflet,
    httr
Suggests:
    testthat,
    knitr,
    tmap
VignetteBuilder: knitr
URL: https://github.com/Robinlovelace/stplanr

• 3. URL for the package (the development repository, not a stylized html page) https://github.com/Robinlovelace/stplanr
• 4. What data source(s) does it work with (if applicable)? Example data on route allocation and travel flows (included - more planned), geographical boundaries, transport networks, example travel survey data (to be added subsequently)
• 5. Who is the target audience? Transport researchers and modellers from academic, public and private sectors. University students studying transport geography, transport modelling and related disciplines.
• 6. Check the box next to each policy below, confirming that you agree. These are mandatory.
• [x] The repository has continuous integration with Travis and/or another service
• [x] The package contains a vignette
• [x] The package contains a reasonably complete readme with devtools install instructions
• [x] The package contains unit tests
• 7. Do you agree to follow the rOpenSci packaging guidelines? These aren't mandatory, but we strongly suggest you follow them. If you disagree with anything, please explain.
• [ ] Are there any packages that your package depends on that are not on CRAN?
• [x] Do you intend for this package to go on CRAN?
• [x] Does the package have a CRAN accepted license?
• [ ] Did devtools::check() produce any errors or warnings? If so paste them below.
• 1. Please add explanations below for any exceptions to the above: This is work in progress and I am learning how to build R packages as I progress. These issues will be fixed, probably during a course on R package development I will attend.
• 9. If this is a resubmission following rejection, please explain the change in cirucmstances.

[sckott]

hi @Robinlovelace - Thanks for your submission.

Did you mean to check off some of those things? E.g., you checked that you have continuous integration, but you don't, and that you have tests, but at least we don't see any.

Can you correct the information above?

[Robinlovelace]

@sckott I need to learn more about 'continuous integration' and tests. I'm attending a 3 day course on R package development in May. Proposal: I close this issue for now and re-open once I know more about those things and have had more feedback about the package in general. And I'll reduce the number of check() warnings! Sound good? Any advice appreciated. The tool is already being used for real-world applications and I'm sure it will be useful, once it's a little more polished.

[sckott]

@Robinlovelace Up to you if you want to close the issue and open a new one later.

Where are users expected to get data for use in functions from your package? If data is publicly available (e.g., via web APIs, or even just ftp), we can help build in some functions to provide data access.

I'm attending a 3 day course on R package development in May.

cool, sounds good!

The R CMD CHECK warnings aren't a huge deal - we can help with those. Bigger issues are:

• fit for ropensci - if there is data access functionality in your package (discussed above), it would be a better fit for us
• vignettes/help for users: this is pretty crucial - we want it to be easy for users to figure out how to use a package
• tests - tests are pretty crucial to make sure any functionality of the package doesn't break when something is changed

[karthik]

@Robinlovelace Just wanted to mentioned that we can also help with some of these issues. If you tag one of us in your repo, we can help with CI, testing and other CHECK issues. And as we go along we can add this to our knowledgebase (on the wiki for this repo) so that others in the future can benefit (and also streamline the process).

[sckott]

@Robinlovelace let's leave this issue open for now. I added the holding tag to indicate that we're in a holding pattern for now, and we can remove again once we are moving forward with a review. Okay with you?

[Robinlovelace]

Sound great @sckott. More detailed responses coming soon, but YES this package enables access to new open source data and YES it will be very well documented with vignettes etc. Check out my work on osmar, which is kind-of an introductory vignette to the package: https://github.com/Robinlovelace/osm-tutorial/blob/master/osm.pdf The reason I want to be affiliated with ropensci is that actively encourages such clear reproducibility, communication and empowerment through technology. Based on the supportive feedback holding sounds like the ideal solution for now.

[sckott]

@Robinlovelace Okay, sounds good. So this package already does data acquisition? Or it will? Do you mind if I uncheck boxes that aren't complete yet?

[Robinlovelace]

@sckott yes. gLines2CyclePath harvests data from the CycleStreet.net routing API. We plan to add interfaces to other routing APIs. Here are a couple of examples of the kind of data it pulls in, of a couple of routes in Manchester and a few thousand OD flows in Coventry:

[Robinlovelace]

Don't mind at all if you uncheck uncompleted boxes, please do.

[sckott]

@Robinlovelace Cool - nice example! Boxed unchecked.

[Robinlovelace]

@sckott OK, I'm down to only 3 warnings from 7.

Any ideas how to remove the remaining 3?

Also check the updated README: https://github.com/Robinlovelace/stplanr

What else do I need in there for this to be 'ropensci compliant'? Thanks for the support, more to come.

[sckott]

WARNING '::' or ':::' import not declared from: ‘sp’ 'library' or 'require' call not declared from: ‘sp’

• Looks like you are using the sp package but you didn't put it into your DESCRIPTION file to load
• Looking at your NAMESPACE file, I'd suggest not using exportPattern("^[[:alpha:]]+") and instead using roxygen @export to explicitly declare what functions you want exported to users, and which not exported - also, using @import tags (e.g., @import sp) will put the appropriate entry in your NAMESPACE file for packages to import

Namespaces in Imports field not imported from: ‘dplyr’ ‘RCurl’ ‘rgdal’ ‘rgeos’ All declared Imports should be used.

• See above comment. Some of these packages are in your DESCRIPTION file, but some are not. R check found that you either A) use one of those packages in yours but it's not imported (in the DESCRIPTION and NAMESPACE files), or B) you don't use one of those packages in your package, but it's listed to import

WARNING LaTeX errors when creating PDF version. This typically indicates Rd problems.

• Hard to say what's wrong here, other than that something with Rd files.

WARNING '::' or ':::' import not declared from: ‘sp’ 'library' or 'require' call not declared from: ‘sp’

• You need to add sp to package imports

[sckott]

Also, I'm getting a bunch more warnings locally running check on your package. Do you not get these?

* using R version 3.1.3 (2015-03-09)
* using platform: x86_64-apple-darwin13.4.0 (64-bit)
* using session charset: UTF-8
* checking for file ‘stplanr/DESCRIPTION’ ... OK
* checking extension type ... Package
* this is package ‘stplanr’ version ‘0.0.0.9005’
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘stplanr’ can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking DESCRIPTION meta-information ... NOTE
License components which are templates and need '+ file LICENSE':
  MIT
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking loading without being on the library search path ... OK
* checking dependencies in R code ... WARNING
'::' or ':::' import not declared from: ‘sp’
'library' or 'require' call not declared from: ‘sp’
'library' or 'require' call to ‘sp’ in package code.
  Please use :: or requireNamespace() instead.
  See section 'Suggested packages' in the 'Writing R Extensions' manual.
Namespace in Imports field not imported from: ‘dplyr’
  All declared Imports should be used.
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... NOTE
disab_recat: no visible binding for global variable ‘b’
gFlow2line: no visible global function definition for ‘Line’
gFlow2line: no visible global function definition for ‘proj4string<-’
gFlow2line: no visible global function definition for ‘proj4string’
gLines2CyclePath: no visible global function definition for ‘Lines’
gLines2CyclePath: no visible global function definition for ‘Line’
gLines2CyclePath: no visible global function definition for
  ‘SpatialLines’
gLines2CyclePath: no visible global function definition for
  ‘proj4string<-’
gLines2CyclePath: no visible global function definition for ‘CRS’
gOverline: no visible global function definition for ‘over’
gOverline: no visible global function definition for
  ‘SpatialLinesDataFrame’
islines: no visible global function definition for ‘gIntersection’
lineLabels: no visible global function definition for ‘coordinates’
lineLabels: no visible global function definition for ‘gCentroid’
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... WARNING
Missing link or links in documentation object 'islines.Rd':
  ‘gOverline()’

See section 'Cross-references' in the 'Writing R Extensions' manual.

* checking for missing documentation entries ... WARNING
Undocumented code objects:
  ‘lineLabels’
All user-level objects in a package should have documentation entries.
See chapter ‘Writing R documentation files’ in the ‘Writing R
Extensions’ manual.
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... WARNING
Undocumented arguments in documentation object 'gOverline'
  ‘fun’

Undocumented arguments in documentation object 'gSection'
  ‘sl’

Undocumented arguments in documentation object 'islines'
  ‘g1’ ‘g2’

Functions with \usage entries need to have the appropriate \alias
entries, and all their arguments documented.
The \usage entries must correspond to syntactically valid R code.
See chapter ‘Writing R documentation files’ in the ‘Writing R
Extensions’ manual.
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... WARNING
'::' or ':::' import not declared from: ‘sp’
'library' or 'require' call not declared from: ‘sp’
* checking contents of ‘data’ directory ... OK
* checking data for non-ASCII characters ... OK
* checking data for ASCII and uncompressed saves ... OK
* checking examples ... ERROR
Running examples in ‘stplanr-Ex.R’ failed
The error most likely occurred in:

> ### Name: gOverline
> ### Title: Convert series of overlapping lines into a route network
> ### Aliases: gOverline
>
> ### ** Examples
>
> data(routes_fast)
> rnet <- gOverline(sldf = routes_fast[1:7,], attr = "length")
Error in gOverline(sldf = routes_fast

[Robinlovelace]

@sckott Yes, it's because I just added gOverline() and related functions which have caused additional warnings - there's been a lot of interest in the package of late and I expect more such functions, created by additional authors, to be added soon. Will debug based on your suggestions.

[Robinlovelace]

@sckott Good news: it's now down to just 2 warnings on my system after the latest commit:

WARNING
'library' or 'require' call not declared from: ‘sp’
See the information on DESCRIPTION files in the chapter ‘Creating R
packages’ of the ‘Writing R Extensions’ manual.

and

WARNING
LaTeX errors when creating PDF version.
This typically indicates Rd problems.

Not sure how to find exactly where the problems are emerging for either one. Any ideas?

Robin

[sckott]

@Robinlovelace still getting more errors than you are, what version of R are you using?

Also, looks like you are still using exportPattern("^[[:alpha:]]+")

[Robinlovelace]

@sckott I was using 3.1.2. Now upgraded to 3.2.0, had to change a few things to avoid errors. Looking at what I should put in the NAMESPACE now. Any pointers, other than the excellent resource on R Packages (Wickham 2015)? The latest log message can be found here: https://github.com/Robinlovelace/stplanr/blob/master/log-latest

Edit:

@export tags now used to generate the NAMESPACE.

[sckott]

@Robinlovelace nice, checked again with your most recent version and got no warnings

[Robinlovelace]

@sckott I now get no warnings also, thanks to this: http://stackoverflow.com/questions/29791500/how-to-identify-latex-errors-in-rd-r-help-files

Next stage: tests with testthat. Any advice for the tests? Can I use the packages own data to run the tests (e.g. cents in stplanr)?

Pointers for 'continuous integration' with Travis also appreciated.

[Robinlovelace]

I've also updated the first comment of this thread to reflect recent improvements in stplanr.

[karthik]

@Robinlovelace Do you have a vignette? If not, just ignore the latex warnings. Those are unrelated to the package itself.

[karthik]

Can I use the packages own data to run the tests (e.g. cents in stplanr)?

Yes!

Re Travis, I'd suggest starting here and we can help from there. http://r-pkgs.had.co.nz/check.html#travis

[jennybc]

Re: using package's own data, here's a tiny example:

https://github.com/jennybc/gapminder/blob/master/tests/testthat/test-gapminder.R

gapminder is just a data package and has only this one test. But data/gapminder.rda is where the data lives within the package and I can just refer to the gapminder data.frame w/o further fuss in my tests. You should be able to do same with your data objects cents, flow, etc.

Re: Travis. If you are using devtools, there is a use_travis() function to set that process in motion. Note: The development version from GitHub has been updated to produce the new, simpler .travis.yml file, as illustrated in the link above. The released CRAN version produces an older, hairier file, FYI.

[Robinlovelace]

Thanks for the information @jennybc, I've now added my first unit test (for gFlow2Line) and it seems to work! I will do the same for the datasets if needs be, but need to prioritise tests for the most important things...

@sckott I'm on the R Packages course taught by Colin Gillespie in Newcastle now. I've added continuous testing with Travis and (after installing r-gdal from the system, not R) it builds successfully! I've also added a vignette and a demo.

Next step that should make the package even more attractive from an rOpenSci perspective is the addition of tools to automatically download and query OSM data: empowering people via data access. When this is implemented, can we move forward by assigning an editor etc?

Finally, by adding a blank line in demo/00Index (randomly!) and adding sp as a dependency (it really is) I've just eliminated all warnings on my system. Wahey!

[sckott]

Nice progress! yes, adding OSM data access would fit better into our wheel-house

[Robinlovelace]

Hi @sckott I think we're ready to move this forward again towards peer review. @richardellison has added functionality for reading in data from official Australian sources and the addition of route_graphhopper() means that we can now use this to access routed osm data from anywhere in the world. E.g. (once you have the graphhopper api key):

#' @examples
#'
#' \dontrun{
#' r <- route_graphhopper("Leeds", "Dublin", vehicle = "bike")
#'
#' library(leaflet)
#'
#' leaflet() %>% addTiles() %>% addPolylines(data = r)
#' }

[sckott]

Cool. So you want to start review?

[Robinlovelace]

Yes. I can polish up the vignette, make sure all is ship shape and create additional 'feature requests' this week. But yes I think now's a good time to get this 'on the road'.

[sckott]

@Robinlovelace I'll do the review myself, and maybe a 2nd one

[sckott]

Super nice package! nice work all.

• Use @importFrom whenever possible. Right now you have import(openxlsx) and import(sp) in your NAMESPACE file. Just import the functions you need. Same for other pkg deps.
• You need to import packages explicitly that you use. You have a bunch of packages in your DESCRIPTION file that are not imported. Looks like your namespacing function calls (e.g., sp::SpatialLinesDataFrame), which is fine if the user happens to have sp installed, but will fail if the user doesn't. Ideally, import only functions you use, and place those imports somewhere in your package (I like to put them all in one file, the pacakge level man file, e.g., stplanr-package.R)
• On check I saw Rd file 'route_cyclestreet.Rd' - make sure line widths for roxygen docs aren't more than 100 characters.
• It appears that XML package is not used at all? If not, remove from DESCRIPTION file.
• Tests: Pleae write tests to cover at least all the major functions before we accept. Use testthat::skip_on_cran() for any tests that do web requests, so that CRAN tests don't fail in case a service is temporarily down
• Title in DESCRIPTION file should be sentence case
• Use a URL field in your DESCRIPTION file: (e.g., URL: https://github.com/ropensci/taxize)
• Curious whether you really need sp in Depends, or if you could have in Imports. Fine if you need in depends, but if you don't absolutely need it there, best not to
• README:
  • At bottom of readme, you say to contact you via email. That's cool, but also direct to the Issue tracker in the repo
  • Point to instructions in readme for installing rgdal and rgeos, they are often painful for folks

• I think token's can be a bit easier for the user. Right now you have e.g.,

  if (!Sys.getenv('CYCLESTREET') == "") {
    cckey <- Sys.getenv('CYCLESTREET')
  }
  if(is.null(cckey)){
    stop("You must have a CycleStreets.net api key saved as 'cckey'")
  }

  However, if someone like me, is using this pkg for the first time, they may not have gotten a key yet, and therefore that first if statement will lead to not creating a cckey variable, which then makes the next if statement fail as it's not found. And I think you should tell them the name of the environment variable to set (CYCLESTREET), not the internal var name cckey. You could chang this up a bit so that the user is given a prompt if a key isn't found, though not necessary

• gOverline for me makes R crash. e.g., this gOverline(routes_fast[c(2, 3, 22),], attrib = "length"). Does that work for you? Looks like it fails here for me (from the gOverline function):

  slu = gSection(sl)
  #> Assertion failed: (!"should never be reached"), function itemsTree, file AbstractSTRtree.cpp, line 371.
  #> Abort trap: 6

• Throughtout package, make sure to have examples that will work for anyone. E.g,. for readTableBuilder() you have examples like readTableBuilder('employmentSLA.csv'), but the user most likely will not have that file. You can put a csv file in inst/examples, though make sure it's small-ish.
• Looks like for web requests you're doing are directly through jsonlite::fromJSON(). We strongly urge you to use httr to do web requests. It provides a better interface for doing web requests, fails better than jsonlite will, and allows users the ability to toggle curl options. Still good to parse the result with jsonlite::fromJSON() though. Let me know if you need help with this.
• Use a code of conduct, see devtools::use_code_of_conduct()
• We strongly prefer snake_case for function names https://github.com/ropensci/packaging_guide#funvar - Would you be up for changing function names?

I'm waiting to get api keys from CycleSteets and GraphHopper - and I'll have more feedback on those functions after I get keys

[Robinlovelace]

Thanks @sckott for this detailed review. I've pushed a load of changes that fix some of the 'quick win' issues. I'll repeat your comments here and strike-through, with comments after, as they get fixed.

• Use @importFrom whenever possible. Right now you have import(openxlsx) and import(sp) in your NAMESPACE file. Just import the functions you need. Same for other pkg deps.

Please take a look at my new package description stplanr-package.R: seem OK?

• On check I saw Rd file 'route_cyclestreet.Rd' - make sure line widths for roxygen docs aren't more than 100 characters.

Should be fixed by https://github.com/Robinlovelace/stplanr/commit/6271997e257e03c5494e741b99077aa331b53998

• It appears that XML package is not used at all? If not, remove from DESCRIPTION file.

True - was a relic from my attempts to parse .osm files: now fixed.

• Tests: Pleae write tests to cover at least all the major functions before we accept. Use testthat::skip_on_cran() for any tests that do web requests, so that CRAN tests don't fail in case a service is temporarily down

This is great advice re. robust tests: I will ask for a public API key for testing the routing data. One problem is that ggmap::geocode seems unreliable so make look for a replacement. This comment is the biggy for me so any suggestions, let me know.

• Title in DESCRIPTION file should be sentence case

I think this fixes the issue: https://github.com/Robinlovelace/stplanr/commit/f4e28a93184987eb99562bf45ee3b9658cefe0ce please let me know.

• Use a URL field in your DESCRIPTION file: (e.g., URL: https://github.com/ropensci/taxize)

Fixed with https://github.com/Robinlovelace/stplanr/commit/2fff807df779c9effe88f6dbaa3e686366bc23ba

• Curious whether you really need sp in Depends, or if you could have in Imports.

Yes this has come up before. I tried to remove it but many things broke: I really think sp is a valid dependency here: a small package that is essential for handling the spatial data classes used by stplanr.

• README: At bottom of readme, you say to contact you via email. That's cool, but also direct to the Issue tracker in the repo

Fixed here: https://github.com/Robinlovelace/stplanr/commit/e7802586325d0accf352e3b1a060f728fa6a4620

• Point to instructions in readme for installing rgdal and rgeos, they are often painful for folks

Fixed here for Ubuntu - suspect it's less of an issue for Windows users but I don't know how to fix this for Mac users: https://github.com/Robinlovelace/stplanr/commit/84aad658db6e4c271e52f953a62dc2277335e8a4

• I think token's can be a bit easier for the user. Right now you have e.g.,

Fixed based on advice from here: https://cran.r-project.org/web/packages/httr/vignettes/api-packages.html

Fix: https://github.com/Robinlovelace/stplanr/commit/a6d8927b67aa477709eafdaae017f62253e250b2

• gOverline for me makes R crash. e.g., this gOverline(routes_fast[c(2, 3, 22),], attrib = "length"). Does that work for you?

Yes it does - not sure what's up with that. Please see image below. What's your session_info?

• Throughtout package, make sure to have examples that will work for anyone.

Agreed - we're on the case! I think all the examples now work, e.g. after: https://github.com/Robinlovelace/stplanr/commit/fdc02d17da58dd84448d397571016e3e9a59d662

• Looks like for web requests you're doing are directly through jsonlite::fromJSON(). We strongly urge you to use httr to do web requests.

This seems like very good advice. I had some strange behaviour from jsonlite so that should help.

Done - please see here: https://github.com/Robinlovelace/stplanr/commit/762ea2eac1e85ac6d4f9a0a843a571442dd10a23

• Use a code of conduct, see devtools::use_code_of_conduct()

Job done: https://github.com/Robinlovelace/stplanr/commit/03272dc7af6f673dc32eef7f1e4927c103ec5672

• We strongly prefer snake_case for function names https://github.com/ropensci/packaging_guide#funvar - Would you be up for changing function names?

So do I and I'm not following my own advice here! Will move to consistent snake_case.

Even though I think gOverline() is a cool name ;) What do you say @barryrowlingson?

Please see fix here: https://github.com/Robinlovelace/stplanr/commit/815afa203b9caf9b2907ee81e21417875fee8a6d

[sckott]

A bit more review:

• The route_cyclestreet() examples failed for me. The ggmap::geocode calls don't work:

  route_cyclestreet(from = "Hereford", to = "Leeds", plan = "quietest")
  #> Information from URL : http://www.datasciencetoolkit.org/maps/api/geocode/json?address=Hereford&sensor=false
  #> Information from URL : http://www.datasciencetoolkit.org/maps/api/geocode/json?address=Leeds&sensor=false
  #> 1: In readLines(connect, warn = FALSE) :
  #>   cannot open: HTTP status was '0 (null)'
  #> 2: In ggmap::geocode(from) :   geocoding failed for "Hereford".
  #>   if accompanied by 500 Internal Server Error with using dsk, try google.
  #> 3: In readLines(connect, warn = FALSE) :
  #>   cannot open: HTTP status was '0 (null)'
  #> 4: In ggmap::geocode(to) :   geocoding failed for "Leeds".
  #>   if accompanied by 500 Internal Server Error with using dsk, try google.

  Is there something simple I'm missing. Do I need auth to use ggmap::geocode. Perhaps you could use geonames package? E.g.,

  geonames::GNpostalCodeSearch(postalcode="M3 4EE")
  #>   adminCode2 adminCode3              adminName3 adminCode1               lng countryCode postalCode adminName1        placeName
  #> 1             E08000003 Manchester District (B)        ENG -2.24808567051586          GB     M3 4EE    England City Centre Ward
  #>           lat
  #> 1 53.4772316872306

• Same for route_graphhopper()

[sckott]

Please take a look at my new package description stplanr-package.R: seem OK?

Yep

One problem is that ggmap::geocode seems unreliable so make look for a replacement. This comment is the biggy for me so any suggestions, let me know.

See my above comment, perhaps geonames

Point to instructions in readme for installing rgdal and rgeos, they are often painful for folks Fixed here for Ubuntu - suspect it's less of an issue for Windows users but I don't know how to fix this for Mac users

e.g, for macs https://github.com/ropensci/geojsonio#install

Any advice re. how to prompt? Should it be a warning that it needs api keys on package load or only on the function calls?

Not on package load. You could also have a separate function to set keys, a wrapper around Sys.setenv

gOverline crashing: Please see image below. What's your session_info?

after loading stplanr

```r
Session info ---------------------------------
 setting  value                                      
 version  R version 3.2.2 Patched (2015-08-14 r69078)
 system   x86_64, darwin13.4.0                       
 ui       RStudio (0.99.703)                         
 language (EN)                                       
 collate  en_US.UTF-8                                
 tz       America/Los_Angeles                        
 date     2015-10-19                                 

Packages --------------------------------------------- 
package  * version date       source                                
 devtools * 1.9.1   2015-09-11 CRAN (R 3.2.0)                        
 digest     0.6.8   2014-12-31 CRAN (R 3.2.0)                        
 lattice    0.20-33 2015-07-14 CRAN (R 3.2.2)                        
 memoise    0.2.1   2014-04-22 CRAN (R 3.2.0)                        
 openxlsx   3.0.0   2015-07-03 CRAN (R 3.2.0)                        
 Rcpp       0.12.1  2015-09-10 CRAN (R 3.2.2)                        
 sp       * 1.2-0   2015-09-08 CRAN (R 3.2.2)                        
 stplanr  * 0.0.1.0 2015-10-16 Github (Robinlovelace/stplanr@0dac505)

[Robinlovelace]

Hi @sckott to provide a quick-fire answer to your question "Perhaps you could use geonames package?" I tested it but found it less effective that google's place search algorithm at finding places with 'fuzzy names' such as "Chapeltown, Leeds, UK". I've replaced ggmap with RgoogleMaps for now as the former seems buggy.

[Robinlovelace]

I think all the outstanding issues except for the unit tests for all functions and the issue with gOverline (now called overline) have now been tackled. Please can you reproduce on other computers - I've tested it and it seems to work.. I've updated the original response message with more cross-outs and comments (see above). I added unit tests for more functions in a commit today, but there are still functions that lack tests. The issues with ggmap have been resolved by use of RgoogleMaps::getGeoCode(). Thanks to your input, R now asks you for your api key interactively if its not set when you use the route_ functions. Please give them a spin!

Many thanks for the review so far.

[sckott]

I tested it but found it less effective

okay, thanks for checking

I'll give it a spin...

[sckott]

@Robinlovelace Tested the routes functions that require keys. Works nicely now!

[Robinlovelace]

Thanks @sckott, really grateful for your suggestions on improving them. I hope to add more routing apis in due course. Perhaps eventually route() could become generic, pulling in route_cyclestreet, route_graphhopper and route_osrm depending on settings.

[sckott]

Sure, and nice work on the package.

Nothing major left I can think of, but installing with building vignette for me turns up an error NOTE: rgdal::checkCRSArgs: no proj_defs.dat in PROJ.4 shared files - which makes the install fail. Do you ever get this error?

[Robinlovelace]

Hi Scott, no I don't get that error message. Following advice from here, I get:

devtools::install_github("robinlovelace/stplanr", build_vignettes = T)
Downloading GitHub repo robinlovelace/stplanr@master
## lots of messages associated with install...
## ...
* DONE (stplanr)
Reloading installed stplanr

The only strange thing was this note but I don't think that's anything to do with stplanr:

Note: the specification for S3 class “AsIs” in package ‘jsonlite’ seems equivalent to one from package ‘RJSONIO’: not turning on duplicate class definitions for this class.

Regarding the ROpenSci process, what's the next stage? Do you recommend I submit it to CRAN?

[sckott]

Nevermind, had to fix my proj installation - there was a problem in most recent proj http://r-sig-geo.2731867.n2.nabble.com/rgdal-1-0-7-td7588869.html

[sckott]

I am getting a segfault though on the call to overline() https://github.com/Robinlovelace/stplanr/blame/master/vignettes/introducing-stplanr.Rmd#L202 Any thoughts?

[Robinlovelace]

I don't get any error on my Linux laptop. Just tried on my Windows work desktop: no issues either.

[sckott]

What version of GDAL/GEOS do you have? I have gdal 1.11.1, geos 3.4.2, proj 4.9.2

[Robinlovelace]

I get

> rgdal::getGDALVersionInfo()
[1] "GDAL 1.11.2, released 2015/02/10"

and

> rgeos::version_GEOS()
[1] "3.4.2-CAPI-1.8.2 r3921"

on both Windows and Linux systems.

[sckott]

Thanks. Other people can't reproduce this bug, so we'll figure it out later if it persists, looks to be just osx, and just me so far :)

Just one more thing on a check I just ran, a few lines too long

Rd file 'calc_catchment.Rd':
  \usage lines wider than 90 characters:
       projection = "+proj=aea +lat_1=90 +lat_2=-18.416667 +lat_0=0 +lon_0=10 +x_0=0 +y_0=0 +ellps=GRS80 +towgs84=0,0,0,0,0,0,0 +units=m +no ... [TRUNCATED]

Rd file 'calc_catchment_sum.Rd':
  \usage lines wider than 90 characters:
       projection = "+proj=aea +lat_1=90 +lat_2=-18.416667 +lat_0=0 +lon_0=10 +x_0=0 +y_0=0 +ellps=GRS80 +towgs84=0,0,0,0,0,0,0 +units=m +no ... [TRUNCATED]

[sckott]

After that, we're ready to bring it in to ropensci.

[Robinlovelace]

Great. This fixes the 90 char limit note - suggest we're ready to go! https://github.com/Robinlovelace/stplanr/commit/7a2cfddb2c20ad1c053de2ac556362f3b6fd4e18

[Robinlovelace]

Planning to submit this to CRAN later this week. If you or anyone has any advice on that, please let me know. Many thanks for this review - it's been great having all out in the open and makes me wonder what benefits open peer review could have for academic publications. I guess ROpenSci will fork this now and I should push to this fork from now on?

[sckott]

I guess ROpenSci will fork this now and I should push to this fork from now on?

I made a team within ropensci org account, you should be able to transfer the repo to ropensci after you accept that invitation (i think)