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/

from software-review.

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

from software-review.

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 👍
• "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

from software-review.

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.

from software-review.

I spent 3-4 hours

from software-review.

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

from software-review.

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

from software-review.

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

from software-review.

Thanks! I've implemented all of these.

from software-review.

Looks good to me 👍

from software-review.

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.

from software-review.

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.

from software-review.

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

from software-review.

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

Any thoughts?

from software-review.

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

from software-review.