Ropenaq package for R

[maelle]

• 1. What does this package do? (explain in 50 words or less)
     It provides an interface to the OpenAQ API. OpenAQ provides open air quality data for many locations around the world, whose number is growing. The R package allows to check data availability and to download measurements from R as a dplyr data.table.
• 2. Paste the full DESCRIPTION file inside a code block (bounded by ``` on either end).

Package: Ropenaq
Type: Package
Title: The Package is a R Interface to the openaq API
Version: 0.1
Date: 2015-12-30
Author: Maëlle Salmon
Maintainer: Who to complain to <maelle.salmon@yahoo.se>
Description: See https://docs.openaq.org/
License: GPL (version 2 or later)
LazyData: TRUE
RoxygenNote: 5.0.1
URL: http://github.com/masalmon/Ropenaq
BugReports: http://github.com/masalmon/Ropenaq/issues
Suggests: knitr,
    rmarkdown
VignetteBuilder: knitr
Depends: httr, lubridate, dplyr, testthat, XML

• 3. URL for the package (the development repository, not a stylized html page)
     https://github.com/masalmon/Ropenaq
• 4. What data source(s) does it work with (if applicable)?
     Open AQ https://openaq.org/#/
• 5. Who is the target audience?
     People that need air quality data for their studies and do not know how to access the API, or want to do the analysis in R anyway. They could be e.g. epidemiologists. I think that if the R package has users, the Open AQ platform will also get more attention and thus get even more data sources.
• 6. Are there other R packages that accomplish the same thing? If so, what is different about yours?
     No, this is the first and only R package for accessing the OpenAQ API (I first asked one of the OpenAQ co-founders)
• 7. Check the box next to each policy below, confirming that you agree. These are mandatory.
• [x ] This package does not violate the Terms of Service of any service it interacts with.
• [ 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
• [ x] The package only exports functions to the NAMESPACE that are intended for end users
• 8. 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.
     Yes, as much as I can.
• Are there any package dependencies not on CRAN?
• [ x] Do you intend for this package to go on CRAN?
  When it is more developped, yes, it could be usefool.
• [ x] Does the package have a CRAN accepted license?
• Did devtools::check() produce any errors or warnings? If so paste them below.
• 9. Please add explanations below for any exceptions to the above:
• 10. If this is a resubmission following rejection, please explain the change in cirucmstances.

[sckott]

@masalmon Thanks for your submission. We are looking for reviewers now.

[maelle]

Ok, thanks for the update!

[sckott]

Reviewers: @aammd @ateucher

[ateucher]

Review

General Comments

• This is a really useful package providing access to an equally useful service that I can see filling a large need. I know quite a few air quality scientists that use R, and I'm sure this will be a big help to them. I'm actually surprised that this is the first package providing this functionality - nice work @masalmon!
• The functions all perform as expected, and from what I can see provide access to the full suite of servcies that the API offers. Overall, the documentation is clear and concise.
• I had a look over at the github page for the OpenAQ API, and it appears that the API is not yet fully mature. This must make developing an R package for it a bit challenging (but also likely helps raise important questions about its functionality and expose hidden bugs so is also probably quite useful in that regard).
• All examples in the README and function documentation run without errors.
• Running devtools::check() turned up five NOTEs, one WARNING, and one ERROR. The error is related to failing tests (see note below), and I think you should be able to address the rest relatively easily from the messages. Many of them are due to files/folders that should be listed in .Rbuildignore (.lintr, .travis.yml, README.Rmd, etc.).
• The vignette doesn't build due to a Gateway Timeout (as noted in Vignette with maps not building ropensci-archive/ropenaq#5)
• develop folder - It's probably a good idea to move this to a development branch and merge features into master as they are developed.

rOpenSci Packaging Guidelines

• Title - To conform to rOpenSci guidelines, you could make the name all lowercase (ropenaq) or even just openaq?
• You've chosen nice, simple, descriptive function names that mirror the openaq API endpoints
• Consider adding a code of conduct to let people know that this is a friendly, open, space to work in :)
• The README is great - nice examples and descriptions. The only thing missing is the title of the package at the top.
• It's generally considered best practice to use Imports: instead of Depends: for dependencies (see https://github.com/ropensci/packaging_guide#-package-dependencies & http://r-pkgs.had.co.nz/namespace.html#imports)

DESCRIPTION file

• Title: I think 'An R package for ...' is frowned upon by CRAN - perhaps something like "Access Air Quality Data From OpenAQ"?
• In the Maintainer: field, put your name (rather than "Who to complain to") :)
• The Description: field should contain one or more complete sentences describing what the package does.
• License: I think the correct specification is GPL (>= 2). @sckott: does ropensci have policies/suggestions on licensing?

Dependencies

• As noted above, I don't think any of your dependencies need to be in the Depends: section. httr, dplyr, and lubridate can all be in Imports:, and from what I can see, XML is not needed at all? testthat should be in Suggests: as it is not necessary to use the package, only to run the tests.
• I feel like OpenStreetMaps (in Suggests:), which depends on rJava, is a pretty heavy dependency just to build the vignette. Can you use ggmap for the mapping in the vignette instead of OpenStreetMaps?

Function Usage and Documentation

• All functions:
  • Value: Should specify that a data.frame ("tbl_df") is returned instead of data.table.
  • I think it would be helpful to specify in a bit more detail what the columns in the returned data frame contain. E.g., locations and count.
• cities:
  • In what format(s) can country be specified? Only the two-letter ISO code as in the example, or will a full name do as well?
• locations:
  • It would be useful to have a bit of an explanation about what combinations of city, country, location are allowed. For example, can location be specified without country and city? What happens if you specify a city that's not in the country specified? Perhaps also reorder to country, city, location so they are in decreasing order of resolution (and the same order as in measurements).
  • It doesn't look like city, country, location, and parameter can take more than one value, though that might be a nice feature. (eg. locations(country = c("IN", "CN"), parameter = c("pm25", "o3"))). I took a quick peek at the API docs and it doesn't look like the API accepts more than one... but could you query the api once for each country and combine them? This could be something to think about for a future version though!
  • date_from and date_to: Perhaps mention what class of inputs these arguments accept - (e.g., Date, POSIXct, POSIXlt, character in the form YYYY-MM-DD?)
• measurements: Similar comments as for location in terms of documentation.
• latest: measurements has a limit param with default value of 100. latest seems like it should have the same for consistency (unless of course the API doesn't have it).

Source Code

General

• I like how thorough you are in your checking of inputs. This will cause the functions to fail informatively when the user inputs something incorrectly.
• Code modularization:

  • Try to write code that is modular to reduce repetetion (the 'DRY' principle: Don't Repeat Yourself). Try to write many more smaller functions that encapsulate discrete operations and that can be reused. Look across the functions and find code that is repeated, and wrap it up as functions to be called within each larger function. For example, you can create a function that returns the base url for all queries:

    base_url <- function() {
        "https://api.openaq.org/v1/"
    }
    
    ## Then you can use that function in each exported function:
    countries <- function() {
        url <- paste0(base_url(), "countries")
        httr::GET(url)
        etc...
    }

    That way if the url changes etc., you only have to change it in one place.

  • It's common practice to put these functions that get used in a lot of other functions in a file called utils.R or zzz.R.

• If you're using the pkg::function() syntax, you don't need to use @import/@importFrom to add these to the NAMESPACE file. As long as the packages are in the Imports: section of the DESCRIPTION, you can use the functions directly using :: without including them in the package namespace. See Hadley's recommendations for this here: http://r-pkgs.had.co.nz/namespace.html#imports. Of course there are many differing opinions on this!

Getting and parsing content

• Use httr::stop_for_status to throw an error when the request fails, and/or use httr::status_code if you want to check for Gateway Timeout (Error code 504) specifically. That way you don't have to grepl for the specific text "Gateway time-out".

• httr::GET can take a named list of arguments to build the query. This can make constructing the call a bit cleaner, so that you don't have to paste0 them all together with &s. You can check the arguments near the top of the function, then construct a list of all arguments to pass to the query argument of GET. For example:

  url <- "https://api.openaq.org/v1/locations?"
  country = "IN"
  city = "Mumbai"
  location = NULL
  arg_list <- list(country = country, city = city, location = location)
  httr::GET(url, query = arg_list) # Any NULL elements of the list supplied to the query paramater are automoatically dropped

• In most (all?) cases you should be able to get the results from page <- httr::GET(query) more easily and directly by using httr::content(page, as = "text"), and then parsing from json to a data frame using jsonlite::fromJSON(), rather than parsing the columns individually with lapply. This should result in more concise code, and hopefully be quite a bit faster.

  • You could write a function to use everywhere for parsing content such as:

  get_content <- function(x) {
    content_type <- httr::headers(x)["content-type"]
    if (!grepl("application/json", content_type)) {
      stop("Unexpected content type")
    }
    cont <- httr::content(x, as = "text")
    parsed_content <- jsonlite::fromJSON(cont, simplifyDataFrame = TRUE, flatten = TRUE)
    parsed_content$results
  }
  
  cont <- get_content(page) ## cont will be a nice data frame

  • If you use the code above in latest(), the measurements column will be a list-column, with each row in the column containing a data frame with columns: parameter, value, lastUpdated, unit. To then expand the data frame returned from get_content() you can use tidyr::unnest_(cont, "measurements"), which will expand the measurements column and fill in the other columns appropriately.

• For dplyr functions, it is probably best to use the non-standard evaluation (NSE) versions of the functions (i.e., mutate_() instead of mutate(). See the vignette as a good resource to get started.

• measurements:

  • I'm not sure you need to specify page=1 or set limit to 100 if it's NULL, as these are the default values for the API.

Tests

• Generally, they look great! There's not a lot you can do test actual values, but you could also add some tests ensuring the dimensions of the returned tables are what you expect - number of columns, and especially number of rows when a limit value is supplied.
• Good job having skip_on_cran for the functions that query the API.
• Best practice is to have one context() call at the top of each file, so you could split the tests into multiple files named test-cites.R, test-countries.R, etc.
• All tests passed for me, except for latest almost always fail with a Gateway Timeout, but as noted this is likely an API issue. You could skip them for now until the API issue is sorted?

This is my first code review, so thanks for letting me take part! Let me know if you have questions or comments; I'm happy to discuss further.

[maelle]

Thanks a lot @ateucher for your thorough and very useful review! I'll start working on this as soon as possible. It will make the package really better, thank you!

I guess there is no devtools function for easily changing the name of the package? I bitterly regret naming it too fast.

[ateucher]

@masalmon it was my pleasure - I really enjoyed doing it :) If there's anything you run into that you need help with, ping me in an issue and/or on twitter. Also, I am definitely not an expert on httr and parsing json content, so if any other rOpenSci people have advice that is better than mine please chime in (@sckott?)

[ateucher]

@masalmon, I should have linked to this in my review: Here's the httr vignette on writing API packages: https://cran.r-project.org/web/packages/httr/vignettes/api-packages.html
Ther's a lot of stuff on authentication that shouldn't apply to this package, but the other tips might be useful.

[maelle]

@ateucher cool, and again, thank you! I hope you'll like the final result. ☺ I'll make a point-to-point answer hopefully next week. I have started improving things but now for some other ones I need to spend time reading (lazy evaluation for instance) -- I am very grateful for the links you put in the review. Regarding httr and jsonlite your advice already made my code much simpler! My old solution was quite clumsy. Have a good week-end!