ropensci-archive/convertr

:no_entry: ARCHIVED :no_entry: Convert Between Units

convertr's Issues

remaining Opensci issues

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

Incorporate wikipedia conversion of units units

Have completed:

length, area, volume

Alternative and unusual units

I was considering submitting a PR to add unusual units to convertr such as "basketball court" and "american football field". Would this be welcome?

Transfer to personal account?

@gshotwell 👋 according to our recently created package curation policy this repo should now be transferred to either one of your personal accounts, or the ropensci-archive organization.

If you don't answer within one month, I'll transfer the repository to ropensci-archive, after which you could still email me to transfer the repo to a personal account.

Thank you!

test that (a + bx) / (c +dx) gives intelligible results

i.e. not 0, NA, negative

Respond to selected text

From the questionr package:

  if (is.null(obj)) {
    if (ifunc_run_as_addin()) {
      context <- rstudioapi::getActiveDocumentContext()
      obj <- context$selection[[1]]$text
      if (obj == "") obj <- NULL
    }

Add test for symbol uniqueness

Symbols need to be unique

Searching for units

There needs to be a way of searching the conversion table so that the user can figure out that "D" is the unit for "Darcy" the area measure of permeable rock. Maybe a lightweight shiny app that would come with the package.