Convertr package: Extensive unit conversion with a shiny gadget.

[gshotwell]

• 1. What does this package do? (explain in 50 words or less)
     Provides extensive unit conversion in R, also includes RStudio add-in to assist the user in picking units.
• 1. Paste the full DESCRIPTION file inside a code block below.

Package: convertr
Type: Package
Title: Convert Between Units
Version: 0.1
Date: 2015-06-24
Authors@R: person("Gordon", "Shotwell", email = "g.e.shotwell@gmaill.com",
                  role = c("aut", "cre"))
Description: Provides conversion functionality between a broad range of
    scientific, historical, and industrial unit types.
Depends:
    R (>= 3.1.0)
Imports: shiny(>=   0.13.2),
    miniUI(>= 0.1.1),
    DT(>= 0.1),
    rstudioapi(>=   0.5)
License: CC0
LazyData: TRUE
BugReports: https://github.com/GShotwell/convertr/issues
Suggests:
    testthat
RoxygenNote: 5.0.1

• 3. URL for the package (the development repository, not a stylized html page)
     https://github.com/GShotwell/convertr
• 4. What data source(s) does it work with (if applicable)?
     None
• 5. Who is the target audience?
     Anyone with units to convert, mostly scientists and commodities analysts.
• 6. Are there other R packages that accomplish the same thing? If so, what is different about yours? datamart provides unit conversion, but with fewer units and a less extensible framework. (It's hard to add units to datamart). convertr has more units. udunits2 Provides the same or greater space of units as convertr, but requires access to an external API. Also udunits2 doesn't have an RStudio add-in. Convertr is based around a built-in conversion table so can be used without an API, it also has an RStudio add-in to assist the user in searching through available units.
• 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 CI and/or another service
     • [ ] 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.

I agree, so long as I can figure out how to use Travis.

* [ ] Are there any package dependencies not on CRAN? 

No * [] Do you intend for this package to go on CRAN?
Yes * [ ] Does the package have a CRAN accepted license?
Yes * [ ] Did devtools::check() produce any errors or warnings? If so paste them below.
No errors

• 9. Please add explanations below for any exceptions to the above:
• 10. If this is a resubmission following rejection, please explain the change in circumstances.
      Not a resubmission

[noamross]

Thanks for the submission @GShotwell! I must say we haven't developed solid criteria about overlap of packages yet. I started a discussion over on the forum about it: https://discuss.ropensci.org/t/overlap-policy-for-package-onboarding/368. I think we'll figure it out soon so we can move forward on this. Please give us your thoughts!

In the meantime, Travis setup is easy and you can use devtools::use_travis() to start setup and you'll find more instructions here: https://docs.travis-ci.com/user/languages/r/

[noamross]

Reviewers: @daattali Due date: 2016-05-24

[daattali]

Review

Overall, a great package that is implemented well and provides clear value. I haven't used the other unit conversion packages, but I can see that convertr is very simple and easy to use and clearly has a very wide range of units available (way more than I knew existed!)

General package guidelines/structure

• Perfect package name
• Good variable naming conventions and syntax
• You could consider adding a CONDUCT.md file; more info
• Since you're using an MIT license, please add a license file; more info
• rOpenSci requires packages to have a NEWS.md file; more info
• Installation and basic usage was smooth and fast with no errors on both Windows and Linux. Tests and R CMD check also ran fine for me locally :+1:
• "data_retrieval" folder: consider renaming this folder to "data-raw" as per Hadley's guidelines
• "data_retrieval" folder: there is a script named "data cleaning.R". It's recommended to not have spaces in file names, consider replacing the space with an underscore

README

• In the first sentence, "cran" should be capitalized to "CRAN"
• In the second sentence, "a numerical vectors" should be "a numerical vector"
• Just a very personal opinion: the examples in the README could just show 1:5 instead of 1:20 so that the README isn't cluttered with so many numbers. 5 is enough to get the point
• The last example preceded by "/don't run" is missing a closing quote in the target unit
• I'm not sure I see the benefit of the "/don't run" in the README. Perhaps replacing that line with a comment saying the following is expected to result in an error is more clear
• After getting approved (or before? @noamross) you should add the rOpenSci footer image to the README; more info

Tests

• There are no tests with vectors (all the tests only use atomic values)
• More tests would be a good idea, 3 positive and 3 negative isn't very comprehensive. For example, there can be a test from unit X -> X, or a test that shows X -> Y == Y -> X
• None of the tests test what happens when a non-numeric is used as the first argument
• Similarly, it might be a good idea to test NULL or NA being passed as different arguments (not mandatory, just to be even more foolproof)

Documentation

• Avoid exporting internal functions, consider using #' @noRd for internal functions (I can see conversion_table.Rd and is_supported_unit.Rd in the "man" folder); more info
• The docs for convert say I can run explore_units() to see what units are available but that function doesn't exist
• Consider adding @return to the convert function documentation
• Data description: when I load the dataframe, I get 1511 rows with 11 variables, not 1283x9 (rp66_symbol and multi_unit seem to be new variables)
• Typo: "fromfrom"
• Typo in roxygen formatting: "#' Dictionary v2.2" is in the middle of a line
• The formula for conversion has a mistake: based on looking at your code, the denominator is C+D*X rather than C+B*X
• I don't know much about that conversion formula - can you ever get a divide by 0 error with the formula? If so, try to catch it. If not, then never mind!
• The examples of the convert function could also benefit from having a dontrun example that shows incompatible types (the README shows this, so it'd be nice to have it in the function examples as well)

Source code

• Generally good design decisions and coding style
• Potential for a helper function: these lines could benefit from a helper get_record(unit). It can also be used in the gadget code
• The req call in the gadget needs to be namespaced to shiny::req

• In the gadget, the code for creating the to/from dropdowns has some repetition and could be simplified by doing something like this (untested code, might not work out of the box):

  units_list <- reactive({ 
  if(input$si_unit == "All") {
    unique(conversion_table$catalog_symbol)
  } else {
    choices <- conversion_table[conversion_table$base_unit == input$si_unit, "catalog_symbol"]
    unique(choices)
  }
  })
  output$from_unit <- shiny::renderUI({ shiny::selectInput("from_unit", "From Unit", units_list())) })
  output$to_unit <- shiny::renderUI({ shiny::selectInput("to_unit", "To Unit", units_list())) })

  Maybe I'm wrong, but I think something like that would work and it's cleaner and less repetitive

• It looks like you're only showing a maximum of 20 records in the table. If that's the case, consider removing the "Showing x out of x entries" (by adding dom = "" to the table's options list)

Functionality

• In convert, when printing the names, consider using a different printing function instead of "print" so that the user won't see the ugly [1] "" and so that the user can have better control over it if it makes sense; more info (if this is purely only for debugging purposes then maybe it's not needed)
• Have you thought about the interaction of saving the results to a variable while printing the names? I think it's a bit weird: consider the command foo <- convert(5, "km", "mi", print_names = TRUE). It will print the origin/target but not the result, and when you print out the variable, only the result will be printed. Have you thought about this and whether this is the desired behaviour?
• Even before seeing the mention of explore_units() I thought something like that would be a very useful addition to your package. How do I know what all available units are? How do I know which conversions are valid? I could see great value in functions for both these questions.

Addin/gadget

• The addin could benefit from better error checking and handling. For example, if I type "1:10" into the numeric vector box, then while I type I see the ugly red Shiny error message and I get lots of errors in my console until the expression is complete. It would be much better user experience to ignore the expression until it is valid or at least catch the error from invalid expressions so that the console isn't flooded with errors and the user will see a more useful error message.
• Similarly, the input should be error checked. Since the gadget has an optional vector as an argument, it'd be a good idea to do some error checking on that input. Running convertr:::convert_gadget("a") or convertr:::convert_gadget(letters) does not do what I expected
• This is more of a personal taste thing, but I think the table looks awkward being restricted to 75% width instead of naturally filling up the entire row
• The UI looks a bit broken since the first two inputs are stuck to the left side without any whitespace, while the next two inputs have some padding. Looks quite weird and broken visually. You can fix this by wrapping all content inside any fluidRow in a column(). Before image and after image
• After choosing a "from" unit , it'd be helpful to only show the relevant "to unit" instead of all available units, where most of them will simply result in an error
• Assuming you end up implementing the functions mentioned earlier that list all available units and all available conversions from a specific unit, then the gadget could have another tab called "Help" or similar that will show ie. what all possible conversions are, the shortname --> fullname map of all units, or any other similarly useful info
• There seems to not be a difference between calling the gadget and running the addin. I feel like this is a bit technically wrong, because gadgets are meant to return a value, not to write to the editor. Addins do that. Renaming all the "gadget" occurrences to "addin" will make it more technically correct, but I think it'd be great to actually support both modes. If you run the app as a gadget, it should simply return a value (that is the intent of gadgets - to be a Shiny app that returns a value that can be stored to a variable), so that you can do something like myvalue <- convertr::convert_gadget(). If you run the addin, then it would run the gadget and in the end use the output value to write it to the document instead of storing it in a variable. This is of course just a suggestion, but I think it would be a useful addition. You can see an example of this kind of functionality in the colourPicker gadget/addin: calling mycol <- shinyjs::colourPicker() runs the gadget which means it will return a value, while running the addin will simply use that return value and write it like you are doing in your code. See the very short colourPickerAddin function to see how simple this can be

Misc

• DESCRIPTION file: there's a tab character in the version number of rstudioapi and shiny

[noamross]

Thanks for your extensive review, @daattali! Could you give an estimate of the hours you spent on it?

@GShotwell let @daattali or I know if you have questions. We aim for responses back to review within two weeks.

[daattali]

I spent 3-4 hours

[gshotwell]

Thanks so much for the amazing comments, I'll get right on them.

[gshotwell]

Thanks again for the great comments, I've learned a lot through this process. My comments are below. Mostly I made the recommended changes, but there were a couple of places where I decided not to, mostly in coding style areas (at what point to define a helper function).

Tests

• I added all these tests

Documentation

All implemented except:

• I removed the data documentation for the conversion table, I moved the table to a sysdata file within the R directory, so think the data documentation is a bit confusing. I added a convenence function to return the table if people are interested in taking a look at it.

Source Code

• I opted not to create the get_record() function because it's slightly below my repetition limit, I think it's a bit harder to read with the helper rather than without.
• Same with the gadget, I think one more repetition and I'd create a new function, but think I'll leave it as is.

Funcitonality

• I took out the print names, I think this function is covered by the add-in/gadget
• explore_units() is added back in

Addin/gadget

• Error checking and messages are improved
• Style changes implemented
• the To unit now reacts to the from-unit selection
• I decided to not add the help menu, explore_units() has a lot of this functionality now, and I'm worried about making a gadget a bit too cluttered.
• I added a return_value argument to the gadget. So that you can either run it to generate an expression, or to do a conversion directly. The main reason I added the gadget along with the add-in was so that the user could start with a particular vector by calling convert_gadget(vector). I can't really think of a good use case where it's helpful to return a converted value rather than the call which generates the value, but I think your right there might be one, so may as well provide an option to return a value.

Misc

• All implemented

[daattali]

Looking much better! I think the new explore_units() is very useful.

I don't have too many new comments so I'll just dump them all together:

• In the README example, you have a type "This will produce and error" --> "This will produce an error"
• I would find it useful to mention in the README that the explore_units() function exists if you want to know what units are available
• Not a big deal, but the recommendation for the data folder is to be named "data-raw" rather than "data_raw"

• In convert.R, if you want to not run an example this is the correct syntax

  #' \dontrun{  
  #' convert(1:20, "kg", "km2")  
  #' }  

• Also in that unrun example notice that there is a missing closing quotation mark
• In the documentation for explore_units() there doesn't seem to be a function title (the title is two sentences, I don't think that's what you meant?)
• Since the data table is now explicitly exported, in my opinion it would make sense to add some documentation about that table and say what each of the variables are. Previously it wasn't exported so it wasn't needed, but now that users are meant to see it, it should be documented
• Convert addin: Every time I select a new "from unit", I momentarily see an error in the gadget, and I see errors in the console. It's be awesome to fix that in order to have a nice smooth user experience without thinking that you did something wrong

[gshotwell]

Thanks! I've implemented all of these.

[daattali]

Looks good to me :+1:

[noamross]

OK, we're about ready to go. Before we transfer this to our repo, @GShotwell, I notice that there's a build error, which looks like R CMD check hanging on running the examples. I believe that this is because functions launching shiny apps should be wrapped in \dontrun{} in examples. Could you fix this? Once there's a clean CI build we should be good to go.

[gshotwell]

Oh drat, I did a check and then thought "hey, let's just add a couple more examples." I've fixed the error and convertr is now passing the CMD check.

[noamross]

Great! OK, next steps:

• Add the footer to your README:

[![ropensci\_footer](http://ropensci.org/public_images/github_footer.png)](http://ropensci.org)

• Go to the Repo Settings --> Transfer Ownership and transfer to ropenscilabs!

Also, if you intend to submit to CRAN:

• I see you have one NOTE, that data-raw is a top-level directory. I believe you want to add this to .Rbuildignore. (Perhaps it used to be called data_retrieval?)
• I recommend using Travis to test against the development version of R. To do so, change sudo to false in your .travis.yml, and adding the following. This will run your tests across multiple R versions:

r:
  - release
  - devel

[gshotwell]

I tried to transfer it, but keep getting this error: You don’t have admin rights to ropenscilabs

Any thoughts?

[sckott]

@GShotwell you should get an invitation to a team on ropenscilabs - then you should be able to do the transfer